Shrink ComputedValue using a bitfield #3880
Conversation
🦋 Changeset detectedLatest commit: ff1d3dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Not that familiar with changesets but this is an implementation detail so I think doesn't need a changeset. Otherwise I'll add one. |
|
@urugator are you able to help with review? Many thanks |
peterm-canva
force-pushed
the
smoosh-computed-value
branch
2 times, most recently
from
c437bf7
to
0177cb0
Compare
|
Added a patchset because @taj-p mentioned I will most likely need that so we can uprev to that version. |
|
Thanks for this PR, this looks great!
I imagine it'd be good to have the release soak for a few weeks, if it doesn't raise any issues sounds good :) |
peterm-canva
force-pushed
the
smoosh-computed-value
branch
from
0177cb0
to
6b15203
Compare
Sounds good, I'll get the next PR ready and we can hold off a few weeks. |
peterm-canva
force-pushed
the
smoosh-computed-value
branch
from
6b15203
to
4504469
Compare
|
There's an issue with this for the production build. The getters/setters are not renamed by minification, but the code that accesses them does get the keys renamed. This means they always come back falsey. Working on that |
peterm-canva
force-pushed
the
smoosh-computed-value
branch
from
4504469
to
ff1d3dd
Compare
| @@ -4,7 +4,7 @@ const utils = require("../../v5/utils/test-utils") | |||
|
|
|||
| const { observable, computed, $mobx, autorun } = mobx | |||
|
|
|||
| const voidObserver = function () { } | |||
There was a problem hiding this comment.
I didn't change this on purpose, I think the formatter is just getting excited because I changed other things in this file
| @@ -22,8 +22,8 @@ export interface IAtom extends IObservable { | |||
| } | |||
|
|
|||
| export class Atom implements IAtom { | |||
| isPendingUnobservation_ = false // for effective unobserving. BaseAtom has true, for extra optimization, so its onBecomeUnobserved never gets called, because it's not needed | |||
There was a problem hiding this comment.
I changed these so that they are no longer mangled (don't end with _). This is so that they still match the new getter/setter names when accessed. Unfortunately the code generated in the final bundle for getters/setters has their name in quotes e.g. "isPendingUnobservation" and name mangling is not smart enough to figure out these are being used as property names.
|
OK, good to go. I had to change names so they aren't mangled any more. See comment in atom.ts. |
|
Can we land this one? |
|
Sorry, missed the update! Merged! |
|
Should soon be available as 6.12.4 @peterm-canva since you are validating performance, I am wondering if you are interested in checking if #3884 also gains these positive results for you? |
Code change checklist
/docs. For new functionality, at leastAPI.mdshould be updatedyarn mobx test:performance)ComputedValueis currently 112 bytes in 64-bit chromium browsers, and it's not unreasonable to see 10s or 100s of thousands of these on very large apps. This makes this class IMO a reasonable target for memory optimization, given small changes can give big wins in absolute memory.This change takes four boolean fields and stores them in one bitfield, adding getters and setters to keep the API to access them the same.
This shrinks 112 bytes down to 100 (verified in devtools) by removing 4x 4 byte booleans and replacing them with 1x 4 byte integer in v8. In Safari the bools take 8 bytes so this should save 3x 8 bytes (but I didn't check).
There will be an increased overhead to get/set the boolean values now given the bit-shifting required. I don't see any regressions running
yarn test:performance, but I'm happy to investigate further if this is a concern. I don't think these booleans are in any really performance critical paths. If they are, I'm pretty confident v8 at least will optimize the bit-shifting well, and only add a few machine instructions in optimized code.Regarding the code, using a bitfield could be done in many different ways, so take what I have here as one suggestion, I'm happy to refactor or make more general. I'd like to avoid introducing a bitfield class, because the allocation of a new object per
ComputedValueinstance will cancel out the memory savings. Better to keep the bitfield as a direct member ofComputedValue.If we can reach a suitable solution here, I'd like to do the same thing to
ObservableValue,Atom,ReactionandObservableObjectAdministrationwhich from a brief glance can all save 12-ish bytes as well.