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.

Sign up for GitHub

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

style: Inline some things that should never ever appear in a profile. #19451

Merged
merged 1 commit into from Dec 1, 2017
Merged

style: Inline some things that should never ever appear in a profile. #19451

merged 1 commit into from Dec 1, 2017

Conversation

emilio
Copy link
Member

@emilio emilio commented Dec 1, 2017 •
edited by SimonSapin

This change is Reviewable

@highfive
Copy link

highfive commented Dec 1, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/declaration_block.rs, components/style/properties/properties.mako.rs
  • @canaltinova: components/style/properties/declaration_block.rs, components/style/properties/properties.mako.rs

@highfive
Copy link

highfive commented Dec 1, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Dec 1, 2017
upsuper
upsuper reviewed Dec 1, 2017
View reviewed changes
components/style/gecko/wrapper.rs
@@ -236,6 +237,7 @@ impl<'ln> GeckoNode<'ln> {
true
}

#[inline]
fn flattened_tree_parent(&self) -> Option<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not particularly convinced this method should be marked inline. It looks non-trivial, especially when flattened_tree_parent_is_parent is inlined. And it is called in more than one places. I would say let's just let compiler decide this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

The whole point of flattened_tree_parent_is_parent is avoiding the normal ool function call, and this method is hot :(

@emilio
Copy link
Member Author

emilio commented Dec 1, 2017

@bors-servo r=xidorn

@bors-servo
Copy link
Contributor

📌 Commit 2bbeda9 has been approved by xidorn

@highfive highfive 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 Dec 1, 2017
@upsuper
Copy link
Contributor

upsuper commented Dec 1, 2017

@bors-servo r=upsuper

@bors-servo
Copy link
Contributor

💡 This pull request was already approved, no need to approve it again.

@bors-servo
Copy link
Contributor

📌 Commit 2bbeda9 has been approved by upsuper

@highfive highfive assigned upsuper and unassigned jdm Dec 1, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 2bbeda9 with merge 1fcaeb0...

bors-servo pushed a commit that referenced this pull request Dec 1, 2017
Auto merge of #19451 - emilio:inlining-is-fun, r=upsuper
1fcaeb0 
style: Inline some things that should never ever appear in a profile.

<!-- 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/19451)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-dev-unit

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 1, 2017
@jdm
Copy link
Member

jdm commented Dec 1, 2017

@bors-servo: retry

  • sccache error

@bors-servo
Copy link
Contributor

⌛ Testing commit 2bbeda9 with merge f7aa22a...

bors-servo pushed a commit that referenced this pull request Dec 1, 2017
Auto merge of #19451 - emilio:inlining-is-fun, r=upsuper
f7aa22a 
style: Inline some things that should never ever appear in a profile.

<!-- 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/19451)
<!-- Reviewable:end -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 1, 2017
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: upsuper
Pushing f7aa22a to master...

@bors-servo bors-servo merged commit 2bbeda9 into servo:master Dec 1, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upsuper upsuper upsuper approved these changes

Assignees

@upsuper upsuper

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@emilio @highfive @bors-servo @upsuper @jdm