-
Notifications
You must be signed in to change notification settings - Fork 3.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
Enable WASM_BIGINT by default #22993
Changes from 38 commits
9eaa392
5532f29
5d13302
152c205
4a6588e
52fedfd
01d5e34
e19febb
2c1b640
c0f2c4a
1f0232b
3cbc37c
b9c8be1
fb50f90
513ce14
8185db1
22165af
dd34e39
166dcc7
3088726
f81e0da
833c212
4004df6
faa22bb
c2627fd
382211e
b05455c
2d5fd55
2cc1703
a8372fc
cb4503e
ed8906a
88775c6
dd73366
b70d46a
2ee2ce3
759de04
f3c20a3
4abe5fb
b7d960a
8a3df8f
d9fb51c
ff8c662
9194564
8e5ba19
02bc462
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
{ | ||
"a.html": 552, | ||
"a.html.gz": 380, | ||
"a.js": 9593, | ||
"a.js.gz": 4230, | ||
"a.wasm": 7615, | ||
"a.wasm.gz": 3471, | ||
"total": 17760, | ||
"total_gz": 8081 | ||
"a.js": 9879, | ||
"a.js.gz": 4288, | ||
"a.wasm": 7580, | ||
"a.wasm.gz": 3449, | ||
"total": 18011, | ||
"total_gz": 8117 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sad that these numbers go up and not down :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Embind didn't support 64bit types before, so now there's some more JS code to handle bigint types. I'm guessing a real world example would show a very small change or improvement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, most of the other changes are going down. And we'll have more improvements to come with the other features. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
{ | ||
"a.html": 552, | ||
"a.html.gz": 380, | ||
"a.js": 6724, | ||
"a.js.gz": 2900, | ||
"a.wasm": 9528, | ||
"a.wasm.gz": 4896, | ||
"total": 16804, | ||
"total_gz": 8176 | ||
"a.js": 7153, | ||
"a.js.gz": 3042, | ||
"a.wasm": 9493, | ||
"a.wasm.gz": 4872, | ||
"total": 17198, | ||
"total_gz": 8294 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
8505 | ||
8357 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
20848 | ||
20328 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
129231 | ||
128974 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,6 @@ | ||
__indirect_function_table | ||
_emscripten_stack_alloc | ||
_emscripten_stack_restore | ||
dynCall_iiiiiijj | ||
dynCall_iiiiij | ||
dynCall_iiiiijj | ||
dynCall_jiji | ||
dynCall_viijii | ||
emscripten_stack_get_current | ||
main | ||
memory |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
8489 | ||
8340 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
20816 | ||
20296 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
128680 | ||
128423 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
9532 | ||
9358 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
24693 | ||
24097 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
171361 | ||
171008 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
8462 | ||
8322 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
20742 | ||
20222 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
142481 | ||
142223 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
8462 | ||
8322 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
20742 | ||
20222 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
145068 | ||
144810 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
8403 | ||
8344 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
20432 | ||
20352 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
121923 | ||
121924 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
9535 | ||
9362 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
24693 | ||
24097 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
232870 | ||
232517 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
8505 | ||
8357 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
20848 | ||
20328 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
132038 | ||
131781 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
3769 | ||
3653 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
8493 | ||
7914 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
169290 | ||
168928 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
7643 | ||
7659 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
18761 | ||
18823 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
2866 | ||
2882 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
6140 | ||
6202 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
8003 | ||
8017 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
21535 | ||
21597 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
14531 | ||
15142 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
2713 | ||
2697 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
6921 | ||
6911 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
2535 | ||
2634 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
2369 | ||
2359 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
4844 | ||
4835 |
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.
I don't follow these changes. This will make the polyfill be added in fewer cases - why is that correct?
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.
The point of the general messing with safari versions ( in feature_matrix itself) is to turn BIGINT on by default without affecting other features. This one here is just so that we won't default to having BIGINT on with this polyfill.
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.
Ok, thanks, I see. So the goal is to split out the safari version change? If so, I wonder if we can do that in this PR? The complexity of splitting it seems relatively high to me.
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.
Well, the real goal is to avoid changing all 3 settings in a single commit. But the commits also tie the settings more directly to the browser versions. So the simplest way to make enabling the features independent from each other (and independent from other consequences of raising the default version target) was to lower the version target for each feature separately (which should be a 2-line change for this feature, and 1-line for the others).
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.
Hmm, sorry, but why was that the real goal? Naively I'd think that adjusting the browser versions would be the natural way to enable features by default, so it fits in this PR, so I'm still missing something, sorry.
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.
Hm. I guess after the release we aren't really bisecting like you do with revisions, we're just testing flags to find out what breaks. So the options are testing by enabling or disabling the feature flags, or by setting the fake MIN_SAFARI version numbers and seeing what changes. But at that point the feature flags will still be there, so it's not clear to me what keeping the fake safari versions adds.
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.
Awesome!!!
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.
Was this in reference to my mention of bisecting on
MIN_SAFARI
?If so then my point there was that if we add Safari 14.7, 14.8 etc., avoiding a single version that adds multiple features, then bisecting
MIN_SAFARI
in the future becomes more useful. I agree that using the feature flags could also be done instead, but that requires one to know which flags are relevant and manually set them up, while bisecting onMIN_SAFARI
can be done by just mindless bisection on a range of numbers. Like mindless bisection on commits, that sounds useful.Anyhow this is a minor advantage of 14.7, 14.8 imo. The larger benefit as it I see it is that once we decide on the 14.7 etc. scheme, it is permanent: we have nothing to revert in a later PR, and we have no odd things that show up during bisection that were temporary before being undone. It is a design that "smooths" out safari versions to make them incremental. If browsers enable multiple features in a single release in the future then the same idea could be used again, that is, I don't see this as a one-off hack: it is a principled approach to this problem, as I see it.
But, again, if no one else sees value in this, that's fine 😄 I'm ok with the alternative, the difference is not huge here.
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.
Yeah, I think mainly where we disagree is that I don't really like the idea of keeping fake browser versions around in the codebase. Bisecting on browser versions isn't really mindless if you have to know that the fake version numbers exist and what they are for, and what they can be set to.
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.
Fair enough, sgtm. Maybe I like the idea of smoothing the history because it sounds like a math operation... but I agree fake versions have the downside you mention too.