Refactor attribute handling to avoid marshalling attributes from Gecko into Servo #11886

bholley · 2016-06-27T23:22:30Z

This marshaling is slow, because Gecko stores attributes as UTF-16 and does not atomize them in all cases, and it turns out that the need for them in Servo is pretty minimal. With some refactoring across servo and rust-selectors we can fix this.

This change is

highfive · 2016-06-27T23:22:33Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/element.rs, components/script_layout_interface/wrapper_traits.rs, components/script/layout_wrapper.rs

highfive · 2016-06-27T23:22:34Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style, layout, and script code, but no tests are modified. Please consider adding a test!

bholley · 2016-06-27T23:23:02Z

This isn't ready for review yet, but I want to test the rust-selectors changes.

@bors-servo try


        Auto merge of #11886 - bholley:attr_refactor, r=<try>

Refactor attribute handling to avoid marshalling attributes from Gecko into Servo This marshaling is slow, because Gecko stores attributes as UTF-16 and does not atomize them in all cases, and it turns out that the need for them in Servo is pretty minimal. With some refactoring across servo and rust-selectors we can fix this.

bors-servo · 2016-06-27T23:23:05Z

⌛ Trying commit b92920d with merge 1bf0fd4...

bors-servo · 2016-06-27T23:26:09Z

💔 Test failed - linux-dev

bholley · 2016-06-28T00:24:17Z

@bors-servo try

bors-servo · 2016-06-28T00:24:19Z

⌛ Trying commit 44ee15e with merge 9bceb20...


        Auto merge of #11886 - bholley:attr_refactor, r=<try>

Refactor attribute handling to avoid marshalling attributes from Gecko into Servo This marshaling is slow, because Gecko stores attributes as UTF-16 and does not atomize them in all cases, and it turns out that the need for them in Servo is pretty minimal. With some refactoring across servo and rust-selectors we can fix this.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11886)

bors-servo · 2016-06-28T00:35:15Z

💔 Test failed - linux-dev

cbrewster · 2016-06-28T00:40:53Z

Tidy failure:

Checking files for tidiness...

./components/script/Cargo.toml:60: Line is longer than 120 characters

./components/style/Cargo.toml:36: Line is longer than 120 characters

./components/layout/Cargo.toml:34: Line is longer than 120 characters

./components/script_layout_interface/Cargo.toml:29: Line is longer than 120 characters

./tests/unit/style/Cargo.toml:16: Line is longer than 120 characters

./ports/geckolib/Cargo.toml:39: Line is longer than 120 characters

bholley · 2016-06-28T00:51:03Z

@bors-servo try

bors-servo · 2016-06-28T00:51:06Z

⌛ Trying commit 8b7b2f7 with merge c16e5d7...


        Auto merge of #11886 - bholley:attr_refactor, r=<try>

Refactor attribute handling to avoid marshalling attributes from Gecko into Servo This marshaling is slow, because Gecko stores attributes as UTF-16 and does not atomize them in all cases, and it turns out that the need for them in Servo is pretty minimal. With some refactoring across servo and rust-selectors we can fix this.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11886)

bors-servo · 2016-06-28T01:42:55Z

💔 Test failed - linux-rel

bors-servo · 2016-06-28T23:24:42Z

☔ The latest upstream changes (presumably #11766) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-07-06T00:50:47Z

💔 Test failed - linux-dev


        Add STYLO_THREADS environment variable to control number of style wor…

…ker threads.


        Add Servo_StyleWorkerThreadCount.


        Regenerate bindings.


        Use each_class rather than get_attr when comparing class lists during…

… style sharing.


        Remove get_attrs from TElement.

This should just be a helper.


        Add has_attr method to TElement.

If this is all the information the caller needs, we can get it from gecko without worrying about atomization and string conversions.


        Add attr_equals to TElement.

Same reasons as the previous patch.


        Upgrade rust-selectors.


        Remove get_attr from TElement. \o/

bholley · 2016-07-06T01:05:01Z

@bors-servo r=SimonSapin

bors-servo · 2016-07-06T01:05:01Z

📌 Commit 187a47d has been approved by SimonSapin

bors-servo · 2016-07-06T01:55:37Z

⌛ Testing commit 187a47d with merge abdf2f2...


        Auto merge of #11886 - bholley:attr_refactor, r=SimonSapin

Refactor attribute handling to avoid marshalling attributes from Gecko into Servo This marshaling is slow, because Gecko stores attributes as UTF-16 and does not atomize them in all cases, and it turns out that the need for them in Servo is pretty minimal. With some refactoring across servo and rust-selectors we can fix this.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11886)

bors-servo · 2016-07-06T02:51:11Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

Manishearth · 2016-07-06T15:04:48Z

I rebased my stylo work to servo master, and it seems like the corresponding commits for this have not yet arrived in the master branch of gecko-dev, or anywhere else.

I found them in https://bugzilla.mozilla.org/show_bug.cgi?id=1283620 -- could we maintain a separate branch which always builds with servo on Github? i.e. whenever you update a bindings file and it lands, just cherry-pick the relevant gecko commits onto that branch. We can sync it when the commit lands in m-c.

bholley · 2016-07-06T19:05:34Z

could we maintain a separate branch which always builds with servo on Github?

Well, it's really a symmetric problem right? If we have a logical changeset that's split across the two repos, it may land first on s/s or m-c. In either case, you need to cherry-pick the corresponding changeset for one repo if you want to build tip on the other. Trying to maintain a "live" revision of one repo that builds with the tip of the other is also fraught - either you need one person always paying attention to every landing, or you need multiple people maintaining the branches, leading to coordination problems.

The best way I've come up with is to maintain two branches which are known to work with each other, which are the stylo branches. This is obviously not great, because sometimes I get behind and don't get the chance to fast-forward the branches and pick up all the changes (like what happened this week). But until we fix the repo problem, I'm not sure there's a better way. Luckily, that's happening now. :-)

Manishearth · 2016-07-07T14:00:24Z

Well, AIUI things land faster in Servo, and we could just hold off on starting the land process in Gecko until it gets r+d in servo. Or vice versa.

The "known good" branches should work if we're syncing enough. At the same time, I'm not very fond of landing changes on a Servo master when the changes can't actually be tested -- this is why I'm proposing an extra stylo_tip gecko-dev branch which everyone puts their commits on, with regular rebases and force-pushes.

highfive assigned larsbergstrom Jun 27, 2016

highfive added the S-awaiting-review label Jun 27, 2016

bholley removed the S-awaiting-review label Jun 27, 2016

highfive added the S-tests-failed label Jun 27, 2016

bholley force-pushed the bholley:attr_refactor branch from b92920d to 44ee15e Jun 28, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jun 28, 2016

highfive added the S-tests-failed label Jun 28, 2016

cbrewster added the S-fails-tidy label Jun 28, 2016

highfive removed the S-tests-failed label Jun 28, 2016

bholley removed S-awaiting-review S-fails-tidy labels Jun 28, 2016

highfive added the S-tests-failed label Jun 28, 2016

highfive added the S-needs-rebase label Jun 28, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 6, 2016

heycam and others added 9 commits May 27, 2016

Add STYLO_THREADS environment variable to control number of style wor…

00fe12d

…ker threads.

Add Servo_StyleWorkerThreadCount.

0417022

Regenerate bindings.

7f2a0a1

Use each_class rather than get_attr when comparing class lists during…

a0c425f

… style sharing.

Remove get_attrs from TElement.

7947afc

This should just be a helper.

Add has_attr method to TElement.

96af00f

If this is all the information the caller needs, we can get it from gecko without worrying about atomization and string conversions.

Add attr_equals to TElement.

364c8e2

Same reasons as the previous patch.

Upgrade rust-selectors.

1d8d1cb

Remove get_attr from TElement. \o/

Loading status checks…

187a47d

bholley force-pushed the bholley:attr_refactor branch from e370584 to 187a47d Jul 6, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jul 6, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 6, 2016

bors-servo merged commit 187a47d into servo:master Jul 6, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 6, 2016

bors-servo mentioned this pull request Jul 6, 2016

stylo: Move all binding-generator logic code to a python script. #12212

Merged

3 of 3 tasks complete

Manishearth mentioned this pull request Jul 8, 2016

Proposed workflow for Stylo (short-term) servo/gecko-dev#1

Open

bholley deleted the bholley:attr_refactor branch Aug 26, 2016

servo / servo

Refactor attribute handling to avoid marshalling attributes from Gecko into Servo #11886

Refactor attribute handling to avoid marshalling attributes from Gecko into Servo #11886

bholley commented Jun 27, 2016 •

edited by larsbergstrom

highfive commented Jun 27, 2016

highfive commented Jun 27, 2016

bholley commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bholley commented Jun 28, 2016

bors-servo commented Jun 28, 2016

bors-servo commented Jun 28, 2016

cbrewster commented Jun 28, 2016 •

edited

bholley commented Jun 28, 2016

bors-servo commented Jun 28, 2016

bors-servo commented Jun 28, 2016

bors-servo commented Jun 28, 2016

bors-servo commented Jul 6, 2016

bholley commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

Manishearth commented Jul 6, 2016

bholley commented Jul 6, 2016

Manishearth commented Jul 7, 2016

servo / servo

Join GitHub today

Refactor attribute handling to avoid marshalling attributes from Gecko into Servo #11886

Refactor attribute handling to avoid marshalling attributes from Gecko into Servo #11886

Conversation

bholley commented Jun 27, 2016 • edited by larsbergstrom

highfive commented Jun 27, 2016

highfive commented Jun 27, 2016

bholley commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bors-servo commented Jun 27, 2016

bholley commented Jun 28, 2016

bors-servo commented Jun 28, 2016

bors-servo commented Jun 28, 2016

cbrewster commented Jun 28, 2016 • edited

bholley commented Jun 28, 2016

bors-servo commented Jun 28, 2016

bors-servo commented Jun 28, 2016

bors-servo commented Jun 28, 2016

bors-servo commented Jul 6, 2016

bholley commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

Manishearth commented Jul 6, 2016

bholley commented Jul 6, 2016

Manishearth commented Jul 7, 2016

bholley commented Jun 27, 2016 •

edited by larsbergstrom

cbrewster commented Jun 28, 2016 •

edited