Skip to content

/ servo

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

Low hanging Dromaeo fruit #6900

Closed
wants to merge 3 commits into from
Closed

Low hanging Dromaeo fruit #6900

wants to merge 3 commits into from
+20 −11

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Aug 2, 2015

I took a look at the profile and fixed some obvious issues.

Didn't have time to get actual performance numbers on this.

r? @jdm

Review on Reviewable
@pcwalton
Copy link
Contributor Author

pcwalton commented Aug 2, 2015

cc @metajack
@pcwalton pcwalton force-pushed the pcwalton:low-hanging-dromaeo-fruit branch from 87cb61a to b031044 Aug 2, 2015
@SimonSapin
Copy link
Member

SimonSapin commented Aug 2, 2015

Doesn’t into_ascii_lowercase emit deprecation warnings? Since we’ve updated past rust-lang/rust#26914.

AsciiExt::make_ascii_lowercase (on &mut String without a return value) has the same functionality, still without copying.
@pcwalton pcwalton force-pushed the pcwalton:low-hanging-dromaeo-fruit branch from b031044 to 7ec37b8 Aug 3, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Aug 3, 2015

Fixed comment, added a couple more perf fixes.
@SimonSapin
Copy link
Member

SimonSapin commented Aug 3, 2015

Looks good to me except that I don’t know what the [Pure] annotations mean.
@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 3, 2015

Are debug_asserts checked in homu builds?

The [Pure] annotations seem to be correct.
@pcwalton
Copy link
Contributor Author

pcwalton commented Aug 3, 2015

They are not presently checked in WPT tests since we started running the tests in release mode. I think we could fix that with Cargo profiles, but I would like to do that as a followup since this is an important performance fix.
@jdm
Copy link
Member

jdm commented Aug 3, 2015

I would like to see Dromaeo before and after numbers for this.
@bors-servo
Copy link
Contributor

bors-servo commented Aug 27, 2015

☔ The latest upstream changes (presumably #7401) made this pull request unmergeable. Please resolve the merge conflicts.
@nox
Copy link
Member

nox commented Aug 31, 2015

servo/saltfs#107
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 17, 2015

@jdm Here's a link with the performance numbers. There seems to be a lot of noise, but it looks positive overall. https://docs.google.com/spreadsheets/d/1CPBJmvTqNEAAGWFQQzfJ8YKNxEQ7AL2wJ2gagt5Td3o/edit?usp=sharing
@pcwalton pcwalton force-pushed the pcwalton:low-hanging-dromaeo-fruit branch from 7ec37b8 to 2f07026 Sep 17, 2015
pcwalton added 3 commits Aug 2, 2015
@pcwalton
script: Switch really expensive O(n)-and-allocating asserts in
6373f55 
attribute getters and setters to be debug-only.

Was killing our Dromaeo performance.
@pcwalton
script: Mark some DOM methods used in Dromaeo as pure.
f6bc285 
We can't LICM them until we are codegen-ing `JSTypedMethodJitInfo` (see
@pcwalton
script: Stop reallocating so much when converting DOM strings to JS
Loading status checks…
2f07026 
values.

`size_hint()` in `utf16_units()` seems busted, so we do it ourselves.
@metajack
Copy link
Contributor

metajack commented Sep 17, 2015

What's the explanation for the 40-50% regressions? I'm not as concerned about the other ones.
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 17, 2015

I'll run again. I suspect it's just that the tests are really noisy.
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 17, 2015

One of the 50% regressions is SunSpider, which pretty much has to be noise because it doesn't touch the DOM at all.
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 18, 2015

@metajack I did another run and updated the spreadsheet.

It looks like Dromaeo is just too noisy in Servo to really draw any conclusions here.
@nox

This comment has been minimized.

Sign in to view
Copy link

nox commented on 2f07026 Sep 20, 2015

size_hint() in utf16_units() isn't busted, the only thing it can assume is that there are at least len / 4 units, AFAICT.
@nox
Copy link
Member

nox commented Sep 21, 2015

Second commit is overseded by #7601.

For the first one, @jdm doesn't want to remove the asserts in release because we can't run the tests in debug mode. What if we introduce an Atom wrapper type signalling that its contents is indeed a valid XML name?

Third commit is nice and could be merged now.
@pcwalton
Copy link
Contributor Author

pcwalton commented Sep 25, 2015

@jdm, do you concur with @nox' assessment here? Would you be willing to merge the third commit?
@nox
Copy link
Member

nox commented Sep 25, 2015

@pcwalton For first commit, you can still improve the asserts themselves by not allocating a lowercase name and traversing it only once.
@jdm
Copy link
Member

jdm commented Sep 25, 2015

Yes, third commit looks fine.
@pcwalton pcwalton mentioned this pull request Sep 27, 2015
Merged
bors-servo pushed a commit that referenced this pull request Sep 27, 2015
bors-servo
Auto merge of #7765 - pcwalton:less-ambitious-low-hanging-dromaeo-fru…
Loading status checks…
7a3d2c8 
…it, r=jdm

script: Stop reallocating so much when converting DOM strings to JS values.

This is split out from #6900.

`size_hint()` in `utf16_units()` seems busted, so we do it ourselves.

r? @jdm

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7765)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented Oct 2, 2015

Closing this PR in favour of future work that eventually allows us to move to debug_assert! without losing confidence that we'll actually detect regressions.
@jdm jdm closed this Oct 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
S-awaiting-review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@pcwalton @SimonSapin @Ms2ger @jdm @bors-servo @nox @metajack @highfive
You can’t perform that action at this time.