Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
DOMEventTarget's constructor ranwm.set(this, new Map). SinceNodeextends it, every element, text node, comment and attribute allocated an emptyMapplus aWeakMapentry, although almost no node in a parsed document ever gets a listener. Both cost memory, and the WeakMap entries are ephemerons, which the garbage collector has to trace separately.The map is now created when the first listener is added.
dispatchEventalready handled a missing entry (invokeListenersstarts withconst map = wm.get(currentTarget); if (map && ...)), so onlyaddEventListener(create on demand) andremoveEventListener(skip when there is no map) needed a line each.This is the single largest effect of the campaign, and it is mostly a garbage-collection effect: in a profile of the extraction workload, GC went from 29% to 6% of samples.
Verification (this branch vs
main)maindocumentElement.cloneNode(true))content.jssequence ondom.html)w3c.html)/pages/*)Retained heap per parsed
w3c.htmldocument, measured by allocating 300 documents and forcing a collection: 675,278 -> 300,888 bytes, -55.4% (this metric is allocation-counting, so it is stable to a fraction of a byte).query-simple(querySelectorAll('div'),getElementsByTagName('p'),getElementsByClassName) reports a regression in the A/B harness, but it is an artefact of loading two module instances into one process: measured standalone, one process per revision, the same workload is equal or faster after the change (13.6 / 16.9 / 12.4 ms before, 10.9 / 11.9 / 14.0 ms after, three runs each). The repo's own benchmark below also showsquerySelectorAll('div')andgetElementsByTagName('p')unchanged.External cross-check with the repo's own
npm run benchmark:html(the 12 MB page), two runs per side. These are standalone runs, not paired, so they carry run-to-run drift; the effects here are far larger than that drift.mainhtml.cloneNode(true)html.innerHTMLround tripquerySelectorAll('div')Observable surface
removeEventListeneron a target that never had a listener now returns without touching a map. Before it read an empty map and found nothing: same result, no throw in either version.WeakMapno longer holds an entry for every node, only for targets that have had a listener added. The map is module-private and not reachable from user code, so this is visible only as lower memory.removeEventListenerremoves the type entry, not the map.addEventListener/removeEventListener/dispatchEventsemantics, includingonce,capture/bubbling order andhandleEventobjects, are untouched.DOMEventTargethas no constructor any more, soEventTarget.prototype.constructoris now the implicit one.new EventTargetkeeps working,instanceofis unchanged.Reproducing the numbers
The harness lives on the campaign branch of my fork:
zirkelc/linkedom@perf/autoresearch. It holdsperf/*.mts(A/B harness, characterisation guard, memory harness, profiler, the 14 cases),perf/plan.md(method, calibration, every experiment and why it was kept or discarded) andperf/experiments.tsv(the log, including the 10 discarded experiments and their numbers).One run takes about 4.5 minutes on an idle machine. Judge a case only when both runs agree; the suite total is the headline number, and
GEOMEANweights every case equally. The fixtures are live pages, so a fresh download changes the absolute milliseconds (not the ratios);perf/fetch-fixtures.shlists the URLs.npm run benchmark:htmlon each side reproduces the external cross-check.Companion PRs from the same campaign, independent of each other and of this one:
Stacked, the four together measure 2.02x on the suite total and -62% on retained heap per document; on
npm run benchmark:html(the 12 MB page) parsing goes from 767/711 ms to 259/235 ms,cloneNode(true)from 591/527 ms to 84/85 ms and the total benchmark time from 4.23/4.09 s to 2.43/2.41 s. Each PR can be taken or left on its own; the numbers in each body are that branch measured alone againstmain.