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

Improve performance of HTMLDivElement constructor #12374

Merged
merged 2 commits into from Jul 10, 2016
Merged

Improve performance of HTMLDivElement constructor #12374

merged 2 commits into from Jul 10, 2016

Conversation

@jdm
Copy link
Member

jdm commented Jul 10, 2016 •
edited by larsbergstrom

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.

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

This change is Reviewable
jdm added 2 commits Jul 10, 2016
@jdm
Update rust-mozjs for cross-crate inlining fixes.
da81464
@jdm
Avoid memmove in HTMLDivElement constructor.
Loading status checks…
d0e3e61
@highfive
Copy link

highfive commented Jul 10, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmldivelement.rs
@highfive
Copy link

highfive commented Jul 10, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@KiChjang
Copy link
Member

KiChjang commented Jul 10, 2016

Are there performance metrics of how much this patch helps with speed?
@Manishearth
Copy link
Member

Manishearth commented Jul 10, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jul 10, 2016

📌 Commit d0e3e61 has been approved by Manishearth
@highfive highfive assigned Manishearth and unassigned SimonSapin Jul 10, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Jul 10, 2016

⌛ Testing commit d0e3e61 with merge 01ec849...
bors-servo added a commit that referenced this pull request Jul 10, 2016
@bors-servo
Auto merge of #12374 - jdm:jsup, r=Manishearth
Loading status checks…
01ec849 
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 -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 10, 2016

💔 Test failed - linux-rel
@nox
Copy link
Member

nox commented Jul 10, 2016

AFAIK the #[allow(unrooted_must_root)] on HTMLDivElement::new can be removed with this change.
@jdm
Copy link
Member Author

jdm commented Jul 10, 2016

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Jul 10, 2016

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...
@bors-servo
Copy link
Contributor

bors-servo commented Jul 10, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@bors-servo bors-servo merged commit d0e3e61 into servo:master Jul 10, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@jdm jdm mentioned this pull request Jul 13, 2016
Open
@bzbarsky
Copy link
Contributor

bzbarsky commented Jul 13, 2016

@jdm do other element constructors need similar changes?
@jdm
Copy link
Member Author

jdm commented Jul 13, 2016

Looks like most elements could benefit from it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Manishearth Manishearth

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@jdm @highfive @KiChjang @Manishearth @bors-servo @nox @bzbarsky @SimonSapin
You can’t perform that action at this time.