Skip to content

Rebaseline code size tests #20530

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Rebaseline code size tests #20530

wants to merge 2 commits into from

Conversation

@RReverser
Copy link
Collaborator

@RReverser RReverser commented Oct 24, 2023

Not sure why they weren't caught by CI (does it skip those tests?), but it looks like there are some differences in unoptimized code size on main.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 25, 2023

Not sure why they weren't caught by CI (does it skip those tests?), but it looks like there are some differences in unoptimized code size on main.

The auto-rollers skip these tests yes.

@RReverser RReverser mentioned this pull request Oct 25, 2023
Merged
@sbc100
Copy link
Collaborator

sbc100 commented Oct 25, 2023

I'm not seeing any deltas on main when I run test/runner other.*code_size* other.*metadce* --rebase... so maybe something is off here with your setup?

@sbc100
Copy link
Collaborator

sbc100 commented Oct 25, 2023

I have llvm built from 77c2b623ca4c and binaryn built from 02e5b160a BTW.. which I think matches the current tot build.

@RReverser
Copy link
Collaborator Author

RReverser commented Oct 25, 2023

I'm not seeing any deltas on main when I run test/runner other.*code_size* other.*metadce* --rebase... so maybe something is off here with your setup?

Sigh, not again...

I cleaned the cache, installed latest emsdk, did ./bootstrap and then did ./test/runner other.*code_size* and this is what I got.

What else am I missing at this point? 😅

@sbc100
Copy link
Collaborator

sbc100 commented Oct 25, 2023

I'm not seeing any deltas on main when I run test/runner other.*code_size* other.*metadce* --rebase... so maybe something is off here with your setup?

Sigh, not again...

I cleaned the cache, installed latest emsdk, did ./bootstrap and then did ./test/runner other.*code_size* and this is what I got.

emsdk tot right?

What else am I missing at this point? 😅

@RReverser
Copy link
Collaborator Author

RReverser commented Oct 25, 2023

What's also interesting (?) is that for this test I'm not seeing a failure.

emsdk tot right?

Yup.

> emsdk install tot 
Fetching latest changes to the branch/tag 'main' for 'C:/Users/me/Documents/emsdk/releases'...
refs/heads/main
Updating 2a590b5d..918d8878
Fast-forward
 DEPS | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Successfully updated and checked out branch/tag 'main' on repository 'C:/Users/me/Documents/emsdk/releases'
Current repository version: "Tue, 24 Oct 2023 22:32:27 +0000 918d8878b1e832bd3a35a888f32c32b15b792c12"
Installing SDK 'sdk-releases-0572a66cd471c367a4892b468d55c10c0a6183af-64bit'..
Skipped installing node-16.20.0-64bit, already installed.
Skipped installing python-3.9.2-nuget-64bit, already installed.
Skipped installing java-8.152-64bit, already installed.
Skipped installing releases-0572a66cd471c367a4892b468d55c10c0a6183af-64bit, already installed.
All SDK components already installed: 'sdk-releases-0572a66cd471c367a4892b468d55c10c0a6183af-64bit'.

@RReverser
Copy link
Collaborator Author

RReverser commented Oct 27, 2023

@sbc100 Did you find why you had different size locally?

@sbc100
Copy link
Collaborator

sbc100 commented Oct 27, 2023

I'm now seeing some differences with main but they are different to the ones you are seeing: #20557

Can you re-run with current tot... i normally just submit these changes directly without review but in this case maybe with worth investigating.

@RReverser
Rebaseline code size tests
4de7671 
Not sure why they weren't caught by CI (does it skip those tests?), but it looks like there are some differences in unoptimized code size on main.
@RReverser
Copy link
Collaborator Author

RReverser commented Oct 27, 2023

Can you re-run with current tot... i normally just submit these changes directly without review but in this case maybe with worth investigating.

Rerunning now... still seeing my differences, and yours in addition (but I actually didn't run metadce tests when making this PR, so they might've been always there).

@RReverser
Copy link
Collaborator Author

RReverser commented Oct 27, 2023

Rebased, reran with latest tot and pushed my metadce changes as well (there is tons of those, seems more than in your PR).

But either way, the original code size difference I saw is still there.

@brendandahl
Copy link
Collaborator

brendandahl commented Oct 27, 2023

I installed tot and ran a rebase against main. I seem to get the same results as Sam. https://gist.github.com/brendandahl/51da397222be9dc64bf336ff09852a47

@RReverser
Copy link
Collaborator Author

RReverser commented Oct 27, 2023

Unless there is still something in my environment despite multiple cleanups and rebases and tot installations this far, I'm getting suspicious of this being somehow a Windows vs Unix issue, even though those tests already set some settings to avoid cross-platform differences.

Does anyone else have a Windows machine to try that unoptimized code size test on?

I'll try on WSL later as well.

@RReverser
Copy link
Collaborator Author

RReverser commented Oct 27, 2023

I guess another thing I could try is ask one of you to upload your output JS from test_unoptimized_code_size so that I could do a diff locally against one produced on my machine.

@brendandahl
Copy link
Collaborator

brendandahl commented Oct 27, 2023

Archive.zip

@RReverser
Copy link
Collaborator Author

RReverser commented Oct 30, 2023

Looks like the difference is indeed in newlines only: my hello_world.js has CRLF and yours has LF endings, but otherwise they're identical.

@RReverser
Copy link
Collaborator Author

RReverser commented Oct 30, 2023 •
edited
Loading

Ah right, the main test_minimal_runtime_code_size test sets '--output_eol', 'linux', but this test doesn't (and neither do some others like metadce).

I'll send a fix.

@RReverser RReverser closed this Oct 31, 2023
RReverser added a commit to RReverser/emscripten that referenced this pull request Oct 31, 2023
@RReverser
Change all size tests to use Linux EOL
5c8852c 
Previously they'd use the EOL of the platform they were run on, which
led to discrepancies on Windows like emscripten-core#20530.
@RReverser RReverser mentioned this pull request Oct 31, 2023
Merged
RReverser added a commit to RReverser/emscripten that referenced this pull request Nov 30, 2023
@RReverser
Leverage coalescing operators. NFC
0e32ac0 
Note that this bumps minimum Chrome & Firefox versions, as well as JS version from ES2020 to ES2021.

I believe this is not blocked on emscripten-core#11984 as it doesn't require Closure to emit runtime helpers.

This is mostly automated with ast-grep, bunch of custom rules, and a few manual fixups. Best viewed with "ignore whitespace changes" (https://github.com/emscripten-core/emscripten/pull/20531/files) due to indentation changes and due to first commit being emscripten-core#20530.

Care was taken to ensure that false-y conditions still use `||` where it might matter and only null-ish conditions use `??`.
RReverser added a commit to RReverser/emscripten that referenced this pull request Nov 30, 2023
@RReverser
Leverage coalescing operators. NFC
2d2f31a 
Note that this bumps minimum Chrome & Firefox versions, as well as JS version from ES2020 to ES2021.

I believe this is not blocked on emscripten-core#11984 as it doesn't require Closure to emit runtime helpers.

This is mostly automated with ast-grep, bunch of custom rules, and a few manual fixups. Best viewed with "ignore whitespace changes" (https://github.com/emscripten-core/emscripten/pull/20531/files) due to indentation changes and due to first commit being emscripten-core#20530.

Care was taken to ensure that false-y conditions still use `||` where it might matter and only null-ish conditions use `??`.
RReverser added a commit to RReverser/emscripten that referenced this pull request Dec 1, 2023
@RReverser
Leverage coalescing operators. NFC
b62683d 
Note that this bumps minimum Chrome & Firefox versions, as well as JS version from ES2020 to ES2021.

I believe this is not blocked on emscripten-core#11984 as it doesn't require Closure to emit runtime helpers.

This is mostly automated with ast-grep, bunch of custom rules, and a few manual fixups. Best viewed with "ignore whitespace changes" (https://github.com/emscripten-core/emscripten/pull/20531/files) due to indentation changes and due to first commit being emscripten-core#20530.

Care was taken to ensure that false-y conditions still use `||` where it might matter and only null-ish conditions use `??`.
RReverser added a commit to RReverser/emscripten that referenced this pull request Dec 7, 2023
@RReverser
Leverage coalescing operators. NFC
cd192bb 
Note that this bumps minimum Chrome & Firefox versions, as well as JS version from ES2020 to ES2021.

I believe this is not blocked on emscripten-core#11984 as it doesn't require Closure to emit runtime helpers.

This is mostly automated with ast-grep, bunch of custom rules, and a few manual fixups. Best viewed with "ignore whitespace changes" (https://github.com/emscripten-core/emscripten/pull/20531/files) due to indentation changes and due to first commit being emscripten-core#20530.

Care was taken to ensure that false-y conditions still use `||` where it might matter and only null-ish conditions use `??`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@RReverser @sbc100 @brendandahl