Bryntum Components are failing to use ICs effectively for GetProperty
Categories
(Core :: JavaScript Engine, task, P3)
Tracking
()
People
(Reporter: mgaudet, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(2 files)
In Bug 1898545 comment 12 and 13 we identified that for some reason this site is not attaching Ion ICs effectively. We should investigate why and potentially add a new IC here.
|
Reporter | |
Comment 1•2 months ago
•
|
||
|
|
Reporter | |
Comment 2•2 months ago
|
||
So I did a pernosco recording of this.
Took some time to jump around and find some stuff. Here's some interesting bits:
- We report we're OK to attach a stub, but it turns out, our stub writer is in a failed state. We marked the stub data as too large here -- our max stub data size is 160 bytes and we've exceeded it here.
Part of the issue here being that we have 37 prototypes on the proto chain to walk. I'll attach what this looks like, but it's definitely an obfuscation tactic issue.
One thing we should do is make it clearer in CacheIR logs when we have stubs which didn't actually attach due to too large. I don't think we can really see that right now.
|
|
Reporter | |
Comment 3•2 months ago
|
||
So, after we fail to attach a stub enough times, the stub goes generic, and we just hang out in ::update the rest of the time.
We see a -lot- of tooLarge stub writes; so I expect this to be a pervasive problem across the page.
|
||
Comment 4•2 months ago
|
||
Walking a long prototype chain isn't the end of the world if we can use shape teleporting. But we disabled shape teleporting for this object because the prototype was modified. If I'm tracing that particular case correctly, it looks like somebody set a $meta property on one prototype object, shadowing the $meta property on another object on the proto chain. That forced us to reshape the shadowed object. Unfortunately it is the holder for the getter for which we want to later attach an IC, but since it disabled shape teleporting, we have to guard the entire proto chain. Then, since the proto chain is too large, we don't attach an IC at all.
Not entirely sure what we could land to fix this. We could increase the max IC size, but we're still going to generate a sequence of 37 proto checks, so that's not going to be particularly fast. Ideally, we wouldn't have to permanently disable shape teleporting here. But I'm not convinced that can be done safely.
A deeply silly option would be to add a GuardPrototypeChain op that owns a list of proto shapes, which would allow us to guard an arbitrarily long proto chain with a single stub field.
The best fix would be for the website to stop modifying prototypes.
I have tried changing how we set the $meta property. Instead of overriding it with:
// At line 4717 in the code shared over email
defineProperty(proto, '$meta', {
value : meta
});
I instead just added a getter on our Base class:
// Just above the removed code above
get $meta() {
return this.constructor.$meta;
}
But that made no noticeable difference. I suppose we might very well be shadowing some more properties elsewhere, not sure how I can spot that. I'll try searching for more defineProperty calls and similar.
|
|
Reporter | |
Comment 6•2 months ago
•
|
||
One thing I would be curious about is how V8 deals with the huge proto chains.
|
|
||
Comment 7•2 months ago
|
||
My best guess here is that the difference between us and V8 is what happens after you've modified a prototype. In both engines, we have to invalidate a bunch of existing shapes. In our case, we also disable shape teleporting for that prototype and fall back on the O(n) path. I think V8's approach (nicely described here) doesn't do that.
A quick and dirty experiment would be to return early from ReshapeForShadowedProp and ReshapeForProtoMutation without setting the HasInvalidatedTeleporting flag. This is profoundly incorrect, but we might be able to get away with it just enough to test performance, which I would expect to be significantly better.
The more correct version of this would be to somehow create a new shape in ReshapeForFoo without setting a flag. The problem is that currently we effectively define (non-dictionary) shapes by-value, so if we're not changing a flag, then we expect to get the same shape back from the initial shapes cache. We might be able to make it work by removing the entry from the cache (and the one-entry cache on its own proto?), but that's a very delicate change that would require careful auditing.
|
|
Reporter | |
Comment 8•1 month ago
|
||
So I did the experiment using this patch, and the results seem pretty good.
These numbers aren't directly comparable to other numbers reported elsewhere
as they were collected on my build machine rather than laptop.
Time to visible also seems to fairly noisy, so take from that what you will.
Baseline numbers:
- generate 8ms
- populating project: 1062ms
- time to visible: 5090ms
- finalize: 1274ms
https://share.firefox.dev/4fu6nVS
With dangerous hack:
- generate: 8ms
- populating project: 606ms (0.57x)
- time to visible: 3848 -- but I will note this number jumps around more than the others across reloads
- finalize: 564ms (0.44x)
https://share.firefox.dev/4gC8uZ6
So for this website it -definitely- seems like we've got room to improve
around shape teleporting.
|
|
Reporter | |
Comment 9•1 month ago
|
||
Also, worth noting, we do see some invalidations in SP3: https://share.firefox.dev/3VNDxZP
|
|
Reporter | |
Comment 10•1 month ago
|
||
(I did experiment if the hack has any impact on SP3 scores; no seems to be the answer)
|
||
Comment 11•1 month ago
|
||
That's a really nice improvement, 🤞 for it being possible to reach a similar gain with a "proper" fix further on.
Re. time to visible, being a fairly long calculation task it yields on a timeout to allow progress reporting in the UI (the green progress bar at the bottom of the time axis). And because of the timeouts it fluctuates more, so that sounds somewhat expected
|
|
Reporter | |
Comment 12•6 days ago
|
||
So I now have telemetry reporting for nightly how often we're seeing the "too large" stub failure to attach. It's right around 1% of documents destroyed.
I'd argue this makes it more interesting for us to attempt a shape teleporting fix, not the least of which because if we are running into teleporting problems frequently and we fix it we'll see it in telemetry.
Description
•