Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructured code to reduce the number of allocations during dispatch. #1

Merged
merged 1 commit into from
Oct 23, 2022

Conversation

nirvdrum
Copy link
Contributor

I use Tree Style Tab with a fairly large number of tabs. I love the extension, but it causes my battery to drain too quickly. I'm sure at a lower number of tabs, this isn't much of a problem. But, I'm profiling it now and trying to fix some stuff.

This set of changes is fairly small, but cuts the retained memory for 18% -> 3% and the allocated memory from 8% -> 2% when I profiled with 25 tabs. I tried to profile under similar conditions and I only focused on "Web Extensions" in the profiler, but there are system variances and workload variances that likely impact it. I ran the profiles multiple times and got similar results each time. But, I'd still take the absolute numbers with a grain of salt and focus more on the relative difference between them. While I didn't focus explicitly on it, it appears from the profiles that GC frequency dropped as well.

The summary of changes are:

  • Eagerly allocate the results array with the correct size to avoid any array resizes
  • Instead of cloning the set of listeners just to map over the array, iterate over the set directly
    • I'm assuming either the iteration order doesn't matter or that the system will consistently iterate over a set the same way it would construct an array from a set. If iteration order is important, we should rethink storing the listeners as a set in the first place
    • This eliminates the array allocation and the data copy
    • This eliminates the allocation of the anonymous function passed to the map call

While I realize this changes from a functional style to a more procedural style, the performance impact is fairly large. I'm open to other approaches as well. I profiled keeping the map but hoisting the function passed to it so we can just pass a function reference rather an allocate a function object. That helps as well and maybe makes the code a bit cleaner, but the benefit was only about half of what this PR proposes.

EventListenerManager.js Outdated Show resolved Hide resolved
@piroor
Copy link
Owner

piroor commented Oct 23, 2022

Thank you for investigation! I agree that my codes are often not optimized well, so optimizations are always welcome for me.

I'll merge this after the first needless part is reverted.

@nirvdrum nirvdrum force-pushed the reduce-dispatch-allocations branch from d51569f to 982415a Compare October 23, 2022 14:55
@piroor piroor merged commit 0be78e7 into piroor:trunk Oct 23, 2022
@piroor
Copy link
Owner

piroor commented Oct 23, 2022

Thanks a lot! I'll update depender addons ASAP.

piroor added a commit to piroor/treestyletab that referenced this pull request Oct 23, 2022
piroor added a commit to piroor/multipletab that referenced this pull request Oct 23, 2022
@nirvdrum nirvdrum deleted the reduce-dispatch-allocations branch October 23, 2022 15:24
piroor added a commit to piroor/tst-bookmarks-subpanel that referenced this pull request Oct 23, 2022
piroor added a commit to piroor/tst-open-bookmarks-as-partial-tree that referenced this pull request Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants