Closed
Bug 1453142
Opened 7 years ago
Closed 1 year ago
Testcase where we're not optimizing DOM things in top-level script with global scope assignment.
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
| Performance Impact | low |
People
(Reporter: bzbarsky, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
It's really simple:
<!DOCTYPE html>
<script>
var start = new Date();
var count = 10000000;
var nodes;
for (var i = 0; i < count; ++i) {
nodes = document.getElementsByClassName("foo");
}
var stop = new Date();
alert((stop - start)/count * 1e6);
</script>
The i++ and nodes assignments should not alias DOM things, so I would expect us to be able to loop-hoist the document.getElementsByClassName() bit and the alert should alert a number in the 1-1.5 (ns/iteration) range. I see numbers closer to 8.
Jan, do you have time to take a look, or know who might?
Flags: needinfo?(jdemooij)
|
||
Comment 1•7 years ago
|
||
It looks like we do optimize the getElementsByClassName call, but we're still doing a getprop in the loop body (see below). This must be the document.getElementsByClassName - we're then guarding that returns the function we expect...
* LInterruptCheck
// i < count
* LoadSlotT
* CompareAndBranch:lt
// Some getprop
* GetPropertyCacheV
* Unbox:Object
* TypeBarrierO
// nodes = ...
* PostWriteBarrierO
* StoreSlotT
// ++i
* AddI
* StoreSlotT
* Goto
|
|
||
Comment 2•7 years ago
|
||
I think the IC code ends up in GetPropIRGenerator::tryAttachDOMProxyUnshadowed. We would need the equivalent of these does-not-shadow checks in IonBuilder, right? Do you know if this used to be inlined?
Bug 1324561 would let us inline this in IonBuilder based on what the Baseline IC did.
|
Reporter | |
Comment 3•7 years ago
|
||
> This must be the document.getElementsByClassName
Oh, hmm...
So one interesting thing is that if I put all this inside a function (which returns the list, so we can't dead-code-eliminate all this stuff) things do get optimized.
I guess that means the global slot assignment is aliasing the getprop, basically? I'm not actually quite sure now how we manage to optimize this in the inside-a-function case.
Depends on: 1324561
|
|
||
Comment 4•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> I guess that means the global slot assignment is aliasing the getprop,
> basically? I'm not actually quite sure now how we manage to optimize this
> in the inside-a-function case.
I mentioned this on IRC yesterday, but for posterity:
<jandem> bz: we use an idempotent IC for this, so in the function case we can loop hoist it I expect. Idempotent ICs only support a few operations, including unshadowed dom proxies
Flags: needinfo?(jdemooij)
|
||
Updated•7 years ago
|
Keywords: perf
Priority: -- → P3
Summary: Testcase where we're not optimizing DOM things as I would expect us to → Testcase where we're not optimizing DOM things in top-level script with global scope assignment.
Whiteboard: [qf]
|
||
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p3:f64]
|
||
Updated•7 years ago
|
Whiteboard: [qf:p3:f64] → [qf:p3]
|
||
Updated•4 years ago
|
Performance Impact: --- → P3
Whiteboard: [qf:p3]
|
||
Updated•3 years ago
|
Severity: normal → S3
|
||
Comment 5•1 year ago
|
||
Nightly: 1.5-2
Chrome: 28-30
Should we close this bug?
Flags: needinfo?(jdemooij)
|
|
||
Comment 6•1 year ago
|
||
(In reply to Mayank Bansal from comment #5)
Nightly: 1.5-2
Chrome: 28-30Should we close this bug?
Yes the issue in comment 1 was likely fixed by Warp.
Status: NEW → RESOLVED
Closed: 1 year ago
Flags: needinfo?(jdemooij)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•