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

style: Get rid of gecko_size_type. #19966

Merged
merged 5 commits into from Feb 7, 2018
Merged

style: Get rid of gecko_size_type. #19966

merged 5 commits into from Feb 7, 2018
+211 −192

Conversation

@emilio
Copy link
Member

emilio commented Feb 6, 2018 •
edited by SimonSapin

It's a hack, should die.

This change is Reviewable
emilio added 3 commits Feb 6, 2018
@emilio
style: Make MozLength / MaxLength a bit more clear and do a bit less …
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
45ba167 
…work.
@emilio
style: Move flex-basis out of gecko_size_type.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
a0d0769
@emilio
style: "%s" % foo is pointless.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
100f4cd
@highfive
Copy link

highfive commented Feb 6, 2018

Heads up! This PR modifies the following files:

  • @bholley: ports/geckolib/glue.rs, components/style/properties/data.py, components/style/values/computed/length.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/position.mako.rs and 2 more
  • @canaltinova: components/style/properties/data.py, components/style/values/computed/length.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/values/specified/length.rs and 1 more
@highfive
Copy link

highfive commented Feb 6, 2018

warning Warning warning

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

emilio commented Feb 6, 2018

r? @nox (pending geckotry)
@highfive highfive assigned nox and unassigned cbrewster Feb 6, 2018
@emilio
Copy link
Member Author

emilio commented Feb 6, 2018

https://treeherder.mozilla.org/#/jobs?repo=try&revision=faa6f3bbe94bc5c5b835d3aa8f02d3bd333de2a7

Is the try run, let's see if I haven't messed up.
@emilio
style: Get rid of gecko_size_type.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
Loading status checks…
32c409d 
This also fixes a style sharing issue with MaxLength, anecdotally.
@emilio emilio force-pushed the emilio:bye-gecko-size branch from 46d1295 to 32c409d Feb 7, 2018
@emilio
Copy link
Member Author

emilio commented Feb 7, 2018

I did indeed mess up, since I overlooked the ToComputedValue implementations. Indeed I even fixed a style sharing issue.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49ad62ceb6430020ebbdc8dba4b24b7b27db347
@nox
Copy link
Member

nox commented Feb 7, 2018

r=me with the green geckotry, nice work.
@emilio
style: Sprinkle some comments.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
Loading status checks…
ed2ba30
@emilio
Copy link
Member Author

emilio commented Feb 7, 2018

@bors-servo r=nox
@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2018

📌 Commit ed2ba30 has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2018

⌛ Testing commit ed2ba30 with merge dc44619...
bors-servo added a commit that referenced this pull request Feb 7, 2018
@bors-servo
Auto merge of #19966 - emilio:bye-gecko-size, r=nox
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
dc44619 
style: Get rid of gecko_size_type.

It's a hack, should die.

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

bors-servo commented Feb 7, 2018

☀️ 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: nox
Pushing dc44619 to master...
@bors-servo bors-servo merged commit ed2ba30 into servo:master Feb 7, 2018
3 of 4 checks passed
3 of 4 checks passed
Taskcluster (pull_request) TaskGroup: failure
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
emilio added a commit to emilio/servo that referenced this pull request Feb 7, 2018
@emilio
style: Fix interpolation of the gecko-size properties.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
d9ae47c 
Followup to servo#19966 because, well...

The animation code should probably be more obvious.
@emilio emilio mentioned this pull request Feb 7, 2018
Merged
bors-servo added a commit that referenced this pull request Feb 7, 2018
@bors-servo
Auto merge of #19969 - emilio:whoopsies, r=emilio
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
cf4f89c 
style: Fix interpolation of the gecko-size properties.

Followup to #19966 because, well...

The animation code should probably be more obvious.
@ionutgoldan
Copy link

ionutgoldan commented Feb 13, 2018

Performance improvements noticed, thanks to this!

== Change summary for alert #11496 (as of Wed, 07 Feb 2018 01:02:13 GMT) ==

Improvements:

7% Stylo Servo_DeclarationBlock_SetPropertyById_Bench osx-10-10 opt 536,227.96 -> 497,745.25
7% Stylo Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench osx-10-10 opt 537,683.83 -> 499,260.92
6% Stylo Servo_DeclarationBlock_SetPropertyById_Bench windows10-64 opt 438,529.71 -> 410,087.83
6% Stylo Servo_DeclarationBlock_SetPropertyById_WithInitialSpace_Bench windows10-64 opt 439,534.08 -> 411,964.83
4% Stylo Servo_DeclarationBlock_SetPropertyById_Bench windows7-32 opt 602,626.50 -> 579,663.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11496
@bholley
Copy link
Contributor

bholley commented Feb 13, 2018

Whoa, that's awesome. @emilio, does that make sense to you? I don't know what gecko_size_type was or what it did.
@bholley
Copy link
Contributor

bholley commented Feb 13, 2018

It's possible that these wins might instead be attributable to #19956, which landed on the same day, and seems more likely to have affected these benchmarks?
@bholley bholley mentioned this pull request Feb 13, 2018
Merged
@emilio emilio deleted the emilio:bye-gecko-size branch Feb 13, 2018
@emilio
Copy link
Member Author

emilio commented Feb 13, 2018

Yeah, this PR removes some extra indirection and branches, but unless the property is width or something like that it should have no effect on that. #19956 seems like a better candidate :)
@emilio
Copy link
Member Author

emilio commented Feb 13, 2018

Actually the properties that those are benchmarking is width so it seems plausible that it's due to this PR.
@nox
Copy link
Member

nox commented Feb 13, 2018

Probably a combination of the two. gecko_size_type existing reduced the opportunities for my PR to do good things.
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
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@emilio @highfive @nox @bors-servo @ionutgoldan @bholley @cbrewster
You can’t perform that action at this time.