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

Jump to bottom

script: Make PartialEq on element type IDs generate a lot less code. #6308

Merged
merged 1 commit into from Jun 13, 2015
Merged

script: Make PartialEq on element type IDs generate a lot less code. #6308

merged 1 commit into from Jun 13, 2015

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Jun 9, 2015

This makes the difference between selector matching scaling on the ARM
Cortex-A9 and not, because the auto-derived PartialEq implementation
blows out the 32KB I-cache. With this change, there is a 2x improvement
in selector matching over sequential when using all 8 cores. (More work
needs to be done; this is a start.)

r? any DOM expert

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 9, 2015
@highfive
Copy link

highfive commented Jun 9, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/5216

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

eddyb
eddyb reviewed Jun 9, 2015
View reviewed changes
components/script/dom/htmltablecellelement.rs
#[allow(unsafe_code)]
fn eq(&self, other: &HTMLTableCellElementTypeId) -> bool {
unsafe {
intrinsics::discriminant_value(self) == intrinsics::discriminant_value(other)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be written in safe code by casting to u8 and comparing that?

@nox
Copy link
Contributor

nox commented Jun 9, 2015

Could we somehow just use something like memcmp?

@pcwalton
Copy link
Contributor Author

pcwalton commented Jun 9, 2015

memcmp will compare structure padding, which might give the incorrect result.

@nox nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 11, 2015
@nox
Copy link
Contributor

nox commented Jun 11, 2015

-S-awaiting-review +S-needs-code-changes

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/script/dom/eventtarget.rs @ r1
  • components/script/dom/htmlelement.rs @ r1
  • components/script/dom/htmlmediaelement.rs @ r1
  • components/script/dom/htmltablecellelement.rs @ r1

components/script/dom/eventtarget.rs, line 84 [r1] (raw file):
Can we cast to u8 here too?

components/script/dom/htmlmediaelement.rs, line 58 [r1] (raw file):
Same remark as eddyb here.

Comments from the review on Reviewable.io

@nox nox self-assigned this Jun 11, 2015
@pcwalton
script: Make PartialEq on element type IDs generate a lot less code.
8c210e1 
This makes the difference between selector matching scaling on the ARM
Cortex-A9 and not, because the auto-derived `PartialEq` implementation
blows out the 32KB I-cache. With this change, there is a 2x improvement
in selector matching over sequential when using all 8 cores. (More work
needs to be done; this is a start.)
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jun 12, 2015
@pcwalton
Copy link
Contributor Author

Removed unsafe code where I could. r? @nox

@nox nox added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 13, 2015
@nox
Copy link
Contributor

nox commented Jun 13, 2015

@bors-servo: r+

-S-awaiting-review +S-awaiting-merge

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/script/dom/htmlmediaelement.rs @ r2
  • components/script/dom/htmltablecellelement.rs @ r2

Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

📌 Commit 8c210e1 has been approved by nox

@bors-servo
Copy link
Contributor

⌛ Testing commit 8c210e1 with merge c3d2425...

bors-servo pushed a commit that referenced this pull request Jun 13, 2015
Auto merge of #6308 - pcwalton:debloat-partialeq, r=nox
c3d2425 
This makes the difference between selector matching scaling on the ARM
Cortex-A9 and not, because the auto-derived `PartialEq` implementation
blows out the 32KB I-cache. With this change, there is a 2x improvement
in selector matching over sequential when using all 8 cores. (More work
needs to be done; this is a start.)

r? any DOM expert

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

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

@bors-servo bors-servo merged commit 8c210e1 into servo:master Jun 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nox nox

Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pcwalton @highfive @hoppipolla-critic-bot @nox @bors-servo @eddyb