perf: Fix edge-case slowdown in compilation #942
Conversation
facebook-github-bot
added
the
CLA Signed
workflow: benchmarks/sizeComparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.
|
packages/babel-plugin/__tests__/stylex-perf-create-theme-test.js
Outdated
Show resolved
Hide resolved
packages/babel-plugin/__tests__/stylex-perf-create-theme-test.js
Outdated
Show resolved
Hide resolved
| const themeFile = path.resolve(__dirname, '../__fixtures__/colorThemes.js'); | ||
|
|
||
| describe('create theme', () => { | ||
| it('transform complex theme file', () => { |
There was a problem hiding this comment.
| it('transform complex theme file', () => { | |
| test('transform complex theme file', () => { |
|
|
||
| function transform(file, opts = defaultOpts) { | ||
| const result = transformFileSync(file, { | ||
| filename: opts.filename || '/stylex/packages/TestTheme.stylex.js', |
There was a problem hiding this comment.
Having a fake default filename here is a bit confusing, esp since we're passing in a "theme" in the test but the fallback is "Theme.stylex.js" which is a file suffix typically used for files containing defineVars exports
| @@ -0,0 +1,48 @@ | |||
| /** | |||
There was a problem hiding this comment.
Can you explain what this test is for? I thought we already had unit tests for theme creation. This is a unit test, so doesn't benchmark anything. If we want a perf benchmark, we should add to the benchmarks instead.
There was a problem hiding this comment.
I added it as a test as I was trying to debug the root cause of the slowdown. It's not quite a benchmark, it's just a verification that the transformation isn't taking an unreasonably long amount of time.
I'm open to suggestions on how exactly to define this. It could be a benchmark, but I'm not sure what that would look like.
There was a problem hiding this comment.
No worries, I'll have a go at converting this into a benchmark next week
| keyPath, | ||
| state.traversalState, | ||
| state.functions, | ||
| state.seen, |
There was a problem hiding this comment.
is the edge case that we weren't properly checking the value of seen here? can we update the summary to include more details
There was a problem hiding this comment.
The edge-case is that we were re-computing objects over and over again previously.
The change adds caching to avoid doing that.
What changed / motivation ?
Identified and fixed a bad edge-case where using certain patterns of constants would take an unreasonable amount of time to compile.
Created a fixture with the problematic code pattern, and added a new jest to to verify that it compiles.
Before the fix, the test that transforms this one file took ~100s on an M1 Max.
After the fix, it dropped to ~1s.
i.e. That is a 100x perf improvement for this particular pattern of code.
TLDR;
Object evaluation wasn't being cached properly.