-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PERF] Class Fields Use Optimization & Made OrderedSet Faster (Again) #7454
Conversation
Asset Size Report for f30c1e6 IE11 Builds EmberData shrank by 1.2 KB (127.0 B compressed)WarningsThe uncompressed size of the package @ember-data/record-data has increased by 584.0 B. Changeset
Full Asset Analysis (IE11)
Modern Builds EmberData shrank by 1.2 KB (127.0 B compressed)WarningsThe uncompressed size of the package @ember-data/record-data has increased by 584.0 B. Changeset
Full Asset Analysis (Modern)
Modern Builds (No Rollup) EmberData shrank by 1.16 KB (159.0 B compressed)WarningsThe uncompressed size of the package @ember-data/record-data has increased by 621.0 B. Changeset
Full Asset Analysis (Modern)
|
a8a11e5
to
f2833d5
Compare
b9a2b0d
to
075bbbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spicy! I like this PR a lot.
@@ -1,22 +1,118 @@ | |||
import { assert } from '@ember/debug'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a unit test for our own OrderedSet now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, but I don't anticipate this lasting more than 2 weeks in the code. We've got a 4.0 deadline to hit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanations! I wasn't aware of the short term-ndess of the changes here as well but this looks great!
Evaluating some of our current perf-based decisions to see if they still ring true.
This started off to see if we can clean up some of our patterns, what I found was inconclusive (which means maybe?) but in the process I also found that using
declare
for class fields that are initialized in the constructor is a nice boost to asset size trimming and also discovered that we were still importing all of Ember to grab GUID_KEY in InternalModel which was no longer used. The latter reminded me that OrderedSet usage was probably de-optimized when we moved to RecordData and sure enough we get a ~4.6% win making sure the set is keyed efficiently again. I also found sadness, but that will hopefully get cleaned up soon.Findings:
declare
has no evident perf impact but significantly improves build size.RecordData
needed GUID_FOR, but I went a step further and ported OrderedSet in and tailored it to useclientId
for now and eliminated our need forguidFor
andGUID_FOR
. This better prepares us for refactoring to identifiers anyway, plus it's actually a good sized win for bundle size.Perf Win
This is purely from having OrderedSet not need to call guidFor on recordData which does not have a GUID_KEY already.