Skip to content
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

[WIP] Bitmask-based change tracking #3945

Merged
merged 46 commits into from Nov 23, 2019
Merged

[WIP] Bitmask-based change tracking #3945

merged 46 commits into from Nov 23, 2019

Conversation

@Rich-Harris
Copy link
Member

Rich-Harris commented Nov 18, 2019

Another crack at #1943. First task is to confirm that it's possible and see how it affects the output; after that, need to figure out how to handle components with more than 31 changeable top-level values

@stalkerg

This comment has been minimized.

Copy link
Contributor

stalkerg commented Nov 19, 2019

how to handle components with more than 31 changeable top-level values

Add another one bitmask? At least, PostgresDB does the same things for nulls in tuples.

@tomcon

This comment has been minimized.

Copy link

tomcon commented Nov 19, 2019

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

Rich-Harris commented Nov 19, 2019

BigInt isn't really an option, it's not supported widely enough. One day. The most likely option at this point is an array of bitmasks

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

Rich-Harris commented Nov 19, 2019

Getting there. All the runtime tests are passing, but there are a few js tests where weird-looking code is being generated. Also, I carelessly broke sourcemaps, which will take a little bit of unpicking.

@Conduitry

This comment has been minimized.

Copy link
Member

Conduitry commented Nov 22, 2019

Sometime in the past when discussing bitmasks, we'd mention having comments next to ctx[123] and 123 & dirty corresponding to the old ctx key names, so that the code isn't so mysterious looking. Is this something it would be possible to add without too many changes? If the code that generates these is fairly localized, it seems like hopefully this would mostly be a question of whether code-red can support something like that.

Rich-Harris added 2 commits Nov 22, 2019
@Rich-Harris

This comment has been minimized.

Copy link
Member Author

Rich-Harris commented Nov 22, 2019

Yes. I've pushed a change to that effect, but... well:

-if (dirty & 2) set_data(t4, ctx[1]);
+if (dirty & /* _id9uocqrtmw00_1_ */ 2) set_data(t4, ctx[1]);

Not quite sure why it's not replacing the comment content — can't seem to reproduce inside code-red.

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

Rich-Harris commented Nov 22, 2019

okay fixed. i think this is basically done now?

@Conduitry

This comment has been minimized.

Copy link
Member

Conduitry commented Nov 22, 2019

Do you think we could use comments where we're accessing ctx[123] as well, and not just where we're doing the bit operation stuff?

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

Rich-Harris commented Nov 22, 2019

yep — done

@Conduitry

This comment has been minimized.

This is very weird.

This comment has been minimized.

Copy link
Member Author

Rich-Harris replied Nov 23, 2019

ugh this is a heisenbug. it happens when i run the js tests in isolation and i have no freaking idea why

This comment has been minimized.

Copy link
Member

Conduitry replied Nov 23, 2019

I've been looking into it in #3977 and it's probably even weirder than you could possibly imagine.

@Conduitry

This comment has been minimized.

Copy link
Member

Conduitry commented on test/js/samples/css-media-query/expected.js in e0eddb5 Nov 22, 2019

This is also very weird. There's more style stuff changing a little farther down too.

@Conduitry

This comment has been minimized.

Copy link
Member

Conduitry commented Nov 22, 2019

It looks like the failing tests are from precisely those mysterious changes to style-related things in a few tests. It seems like something weird happened when you ran the tests locally to update the expected js samples? I have no idea.

@Conduitry

This comment has been minimized.

Copy link
Member

Conduitry commented Nov 22, 2019

Pushed an update. I'm at a loss as to what could've happened. The tests pass locally for me now - hopefully they will here as well.

@Conduitry

This comment has been minimized.

Copy link
Member

Conduitry commented Nov 23, 2019

#3977 was actually responsible for the weird expected.js changes here. If the very first thing the compiler compiles doesn't have component styles, weird stuff happens. During CI, this is normally hidden, since css is alphabetically first, and so the first test that is run does have component styles.

I bet when you were running the tests locally just before committing the changes to the expected.js files, you using describe.only to only run the js tests, or were otherwise doing something that caused the first compiled component to not have component styles.

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

Rich-Harris commented Nov 23, 2019

Good news — I just rebuilt svelte.dev with this branch, and the combined weight of all the minified JS chunks is 3.8% smaller. Zipped, the difference is less (1.3%), but still not nothing. Given how much of the weight of the site is accounted for by CodeMirror, I'm encouraged by these numbers.

I haven't done any performance testing, but based on microbenchmarks of object lookups vs bitwise operations, this is basically guaranteed to be faster, especially since we're no longer constantly changing the shape of the changed object. And each component will use slightly less memory as well. Win all round.

I'll merge this in, and then maybe start testing another hypothesis: since it's now nice and easy to say 'everything changed' (by setting the bitmask to -1), it may be possible to unify initial creation and update, which would remove a lot of duplicated code. Not totally straightforward — need to think about transitions, and how to create a component without running its <script> block until it first gets props — but worth exploring.

@Rich-Harris Rich-Harris merged commit cd21acf into master Nov 23, 2019
20 checks passed
20 checks passed
Tests (8, ubuntu-latest)
Details
Tests (8, ubuntu-latest)
Details
Tests (8, windows-latest)
Details
Tests (8, windows-latest)
Details
Tests (8, macOS-latest)
Details
Tests (8, macOS-latest)
Details
Tests (10, ubuntu-latest)
Details
Tests (10, ubuntu-latest)
Details
Tests (10, windows-latest)
Details
Tests (10, windows-latest)
Details
Tests (10, macOS-latest)
Details
Tests (10, macOS-latest)
Details
Tests (12, ubuntu-latest)
Details
Tests (12, ubuntu-latest)
Details
Tests (12, windows-latest)
Details
Tests (12, windows-latest)
Details
Tests (12, macOS-latest)
Details
Tests (12, macOS-latest)
Details
Lint
Details
Lint
Details
@Rich-Harris Rich-Harris deleted the bitmasks branch Nov 23, 2019
This was referenced Nov 24, 2019
@RedHatter

This comment has been minimized.

Copy link
Contributor

RedHatter commented Dec 1, 2019

These changes look great! Smaller bundle size and better performance. They do cause an issue with svelte-devtools though. Now that the ctx object is a simple array I have no way to map the values back to their original names. For example, previously a user might see something like this

State
string: "testing"
number: 5

Now in 3.16 they would instead see

State
0: "testing"
1: 5

I'm not sure how to solve this. Could some sort of ctx index to local name map be exposed in dev mode, similar to how the props work?

Devtools is still "usable" you can still see and change the state it's just completely unnamed. Let me know if you have any ideas.

@Rich-Harris

This comment has been minimized.

Copy link
Member Author

Rich-Harris commented Dec 2, 2019

@RedHatter I think that's the right approach, yeah (a lookup provided in dev mode). Mind raising an issue?

(for future reference: I don't get email notifications of activity on issues and PRs, so comments on stuff that's closed/merged are very likely to pass me by. I only happened to see this because of an ancient open tab 😀 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.