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

document.createElement very slow. #12354

Open
samuknet opened this issue Jul 8, 2016 · 27 comments
Open

document.createElement very slow. #12354

samuknet opened this issue Jul 8, 2016 · 27 comments

Comments

@samuknet
Copy link
Contributor

@samuknet samuknet commented Jul 8, 2016

document.createElement appears to be much slower than in FF/Chrome.

The core issue is shown with this code:

function test(n) { // Create n 'div' elements using document.createElement
  var then = performance.now();
  var str = 'div';
  for (var i = 0; i < n; i++) {
    document.createElement(str);
  } 
  return performance.now() - then;
}
// Run test 1000 times and average
var times = [];
for (var i = 0; i < 1000; i++) {
  times.push(test(1000));
}
console.log(times.reduce((acc, n) => acc + n) / times.length); // Print avg. time

FF/Chrome avg. time around 0.317465..
Servo avg. time around 1.3

Full testcase here.

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jul 8, 2016

I can repro these numbers

@metajack
Copy link
Contributor

@metajack metajack commented Jul 8, 2016

Since we create a reflector eagerly and Firefox does it lazily, I would like to see numbers where you modify the benchmark such that the reflector would be created in Firefox. Then at least we will know whether this is the cost of eager creation or if there is another problem hiding here.

@bzbarsky What is the simplest way to ensure a reflector is created? Just access some property of the element?

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Jul 8, 2016

Since we create a reflector eagerly and Firefox does it lazily

Not relevant here. createElement has to return the reflector, obviously, so it has to create it in Firefox. This is an honest apples to apples comparison.

Note that it would be interesting to test the no-reflector-creation case too (using cloneNode(true) on a tree of nontrivial size). I seem to recall @wycats saying that skipping reflector creation in Firefox helps a lot there, and we were wondering what the Servo numbers would look like. But that's a separate issue, obviously.

@jdm
Copy link
Member

@jdm jdm commented Jul 8, 2016

So far profiling has revealed a stray memmove in the div element constructor, but it's been difficult to pin down what's causing it.

@jdm
Copy link
Member

@jdm jdm commented Jul 8, 2016

Inlining the box expression in HTMLDivElement::new into the Node::reflect_node call made the memmove go away, but the timing did not noticeably improve. Current profile shows 25% of time spent under HTMLDivElement::new (14% under HTMLElement::new, 10% under Element::new_inherited, 3.5% under EventTarget::new_inherited), but no smoking guns :(

@jdm
Copy link
Member

@jdm jdm commented Jul 8, 2016

Following @bzbarsky's advice, I ran the following test in both servo and gecko (with some modifications to the TextEncoder interface):

<script>
var t = 'TestBinding' in window ? (new TestBinding()) : (new TextEncoder());
var start = new Date();
var count = 1000000;
for (var i = 0; i < count; i++) {
  var a = t.receiveVoid();
}
var stop = new Date();
console.log('void method: ' + ((stop - start) / count * 1e6) + 'ns');

var start = new Date();
var count = 1000000;
for (var i = 0; i < count; i++) {
  var a = t.longAttribute;
}
var stop = new Date();
console.log('int getter: ' + ((stop - start) / count * 1e6) + 'ns');

var n = document.firstElement;
var start = new Date();
var count = 1000000;
for (var i = 0; i < count; i++) {
  var a = n.firstChild;
}
var stop = new Date();
console.log('firstChild getter: ' + ((stop - start) / count * 1e6) + 'ns');                   
</script>

Results:

Gecko:
void method: 8ns
int getter: 9ns
firstChild getter: 16ns

Servo:
void method: 39ns
int getter: 33ns
firstChild getter: 74ns
@jdm
Copy link
Member

@jdm jdm commented Jul 8, 2016

With https://gist.github.com/jdm/4755af850d2d718aac3fc95da456556f applied to rust-mozjs I get void method timings of 18-20ns. The profile shows 30% of execution time is spent code related to catching panics, courtesy of my changes in #11803. I guess we need to talk to the Rust team about what can be done to reduce that.

@jdm
Copy link
Member

@jdm jdm commented Jul 8, 2016

I filed rust-lang/rust#34727 about catching panics hurting our performance.

@jdm
Copy link
Member

@jdm jdm commented Jul 8, 2016

I tried removing the catch_unwind stuff and measuring again, and I get timings around 10ns.

@jdm
Copy link
Member

@jdm jdm commented Jul 8, 2016

Curiously enough, I tried measuring the firstChild getter with the inlining changes and catch_unwind removed - the first two runs averaged 4ns and 2ns, and every single run after that averaged 42-48ns. I have no idea what to make of that. This is consistent across restarts of Servo and page reloads, too.

@wycats
Copy link

@wycats wycats commented Jul 8, 2016

I seem to recall @wycats saying that skipping reflector creation in Firefox helps a lot there, and we were wondering what the Servo numbers would look like. But that's a separate issue, obviously.

An easy way to test this is to compare constructing a tree using cloneNode with an equivalent tree constructed using the DOM APIs.

@jdm
Copy link
Member

@jdm jdm commented Jul 8, 2016

No surprise given #12358, the profile for the firstChild getter is dominated by JSCompartment::wrap and related SM wrapping functions.

bors-servo added a commit to servo/rust-mozjs that referenced this issue Jul 8, 2016
Enable cross-crate inlining for JSVal conversion methods.

Make it possible for Rust methods which are called in the generated bindings to be inlined. This shaved off 1/3-1/2 of the total time in the measurements in servo/servo#12354.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/276)
<!-- Reviewable:end -->
@notriddle
Copy link
Contributor

@notriddle notriddle commented Jul 9, 2016

So half the problem is insufficient inlining (a solved problem), and the other half is panic catching?

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Jul 9, 2016

And the third half is the slow compartment wrapping. There might be other things too; those were just the biggest offenders so far...

@jdm jdm mentioned this issue Jul 10, 2016
3 of 3 tasks complete
bors-servo added a commit that referenced this issue Jul 10, 2016
Improve performance of HTMLDivElement constructor

These changes address two sources of performance loss seen while profiling in #12354. #12358 and rust-lang/rust#34727 are still the biggest offenders, however.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because we don't have performance tests and these are only optimizations

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12374)
<!-- Reviewable:end -->
@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jul 11, 2016

FTR, servo/rust-mozjs#276 landed

@jdm
Copy link
Member

@jdm jdm commented Jul 13, 2016

Sadly #12374 and #12395 have only shaved 90ms off of the original testcase for me so far :(

bors-servo added a commit to servo/rust-mozjs that referenced this issue Jul 13, 2016
More inlining possibilities

These methods were showing up in profiles in servo/servo#12354. This change reduced testcase time by 100ms.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/277)
<!-- Reviewable:end -->
@jdm jdm mentioned this issue Jul 13, 2016
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jul 13, 2016
Enable more DOM bindings inlining opportunities.

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix (partially) #12354
- [X] These changes do not require tests because no automated performance tests
bors-servo added a commit that referenced this issue Jul 15, 2016
Add a manual test for measuring DOM binding performance

This is the test harness I've been using for profiling and measuring in #12354.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12449)
<!-- Reviewable:end -->
@jdm
Copy link
Member

@jdm jdm commented Aug 22, 2016

Servo master reports ~1.0 for me now. Progress!

@metajack
Copy link
Contributor

@metajack metajack commented Aug 22, 2016

@jdm So that's still just over 3x slower?

@jdm
Copy link
Member

@jdm jdm commented Aug 22, 2016

Firefox gives me ~0.4, so 2-3x.

@jdm
Copy link
Member

@jdm jdm commented Aug 22, 2016

Correction, with LTO enabled I get ~0.9.

@jdm
Copy link
Member

@jdm jdm commented Aug 22, 2016

As far as the timings for tests/html/bindings_perf.html, the situation compared with Gecko is much better:

Gecko:
void method: 8ns
int getter: 9ns
firstChild getter: 16ns
Servo:
void method: 9.215ns
int getter: 8.6575ns
firstChild getter: 18.425ns

This might only need #12357 to make a big difference for the testcase here.

@jdm
Copy link
Member

@jdm jdm commented Aug 22, 2016

Unfortunately, I still get 0.8-0.9 with #12980 applied.

@jdm
Copy link
Member

@jdm jdm commented Sep 25, 2016

My servo time is now ~0.6. Progress!

@nox
Copy link
Member

@nox nox commented Sep 25, 2016

What changed?

@jdm
Copy link
Member

@jdm jdm commented Sep 26, 2016

The various changes to rustc linked from rust-lang/rust#34727.

@nox
Copy link
Member

@nox nox commented Oct 4, 2017

This still seems to be an issue.

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 24, 2019

WPT tests that do a lot of element-creating certainly seem to have timeout and other slow-test problems, e.g. #6386 and #25142

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

Successfully merging a pull request may close this issue.

None yet
10 participants
You can’t perform that action at this time.