remove extra clones from dom event script #14066

ducks · 2016-11-04T15:02:28Z

fixes #14062 by removing extra clone calls

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14062 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because elements are already copies

This change is

highfive · 2016-11-04T15:02:32Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/event.rs
@KiChjang: components/script/dom/event.rs

highfive · 2016-11-04T15:02:33Z

Warning

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

nox

Thanks for your contribution! Just one nit.

nox · 2016-11-04T15:03:42Z

components/script/dom/event.rs

-        let bubbles = self.bubbles.clone();
-        let cancelable = self.cancelable.clone();
+        let bubbles = self.bubbles;
+        let cancelable = self.cancelable;


Nit: pass self.bubbles and self.cancelable directly instead of binding them to local variables.

I moved them out because the borrow check was complaining that self was moved because of self.name in the next line.

Relevant rustc issue: rust-lang/rust#16223

ducks · 2016-11-04T18:25:19Z

So I tried doing that but am getting errors with self being moved.

https://gist.github.com/rjgoldsborough/7146daab82378135a37e79ee3be5aee1

Doing a clone on that self.name first seems to fix it but dunno if that's much better?

heh yeah, what @frewsxcv just said :)

ducks · 2016-11-04T20:20:19Z

I pushed up a commit that clones the self.name.

Lemme know if there is a better fix and I will try to apply it.

frewsxcv · 2016-11-04T20:23:39Z

We probably don't want to clone self.name. Cloning a type that implements Copy (self.bubbles and self.cancelable both implement Copy) is free. So, from what I understand, these are the same, performance wise:

let bubbles = self.bubbles.clone();

let bubbles = self.bubbles;

Cloning an Atom is cheap (just an atomic refcount bump), but it's not as cheap as cloning a Copy-type. So we should be fine with just your first commit.

ducks · 2016-11-04T20:27:15Z

Ah ok, cool. Rebased that commit out and repushed.

Thanks.

frewsxcv · 2016-11-04T20:27:47Z

Thanks! 🎊

@bors-servo r+

bors-servo · 2016-11-04T20:27:49Z

📌 Commit 5b22f85 has been approved by frewsxcv

frewsxcv · 2016-11-04T20:28:30Z

For more info about Copy, there's a bit of documentation here.

ducks · 2016-11-04T20:36:53Z

Thanks much

bors-servo · 2016-11-04T23:24:39Z

⌛ Testing commit 5b22f85 with merge bd2fae6...


        Auto merge of #14066 - rjgoldsborough:remove-extra-clones-14062, r=fr…

…ewsxcv remove extra clones from dom event script  fixes #14062 by removing extra clone calls ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14062 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because elements are already copies   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14066)

bors-servo · 2016-11-05T00:09:34Z

💔 Test failed - linux-rel-wpt

highfive · 2016-11-05T00:09:37Z

  ▶ FAIL [expected PASS] /_mozilla/css/inline_block_opacity_change.html
  └   → /_mozilla/css/inline_block_opacity_change.html aed6845267bba6c52d3aa69c4889449b454d8117
/_mozilla/css/inline_block_opacity_change_ref.html febec1b4730afab195f280694dad47200b09ea3c
Testing aed6845267bba6c52d3aa69c4889449b454d8117 == febec1b4730afab195f280694dad47200b09ea3c

nox · 2016-11-05T00:10:30Z

@bors-servo retry #13360

highfive · 2016-11-05T15:01:17Z

  ▶ TIMEOUT [expected CRASH] /_mozilla/mozilla/sigsegv.html
  │ 
  │ VMware, </span><span class="stdout">Inc.
  │ Gallium 0.4 on softpipe
  └ 3.3 (Core Profile) Mesa 12.0.1

frewsxcv · 2016-11-05T15:09:11Z

@bors-servo retry #14067

No longer applicable

ducks · 2016-11-05T15:15:03Z

Looking like this is a build thing but let me know if I can do anything.

frewsxcv · 2016-11-05T15:17:19Z

No worries, you're not doing anything wrong, your changes are good. When running tests, Servo (unfortunately) encounters intermittent failures/passes, so we have to 'retry' builds regularly.

bors-servo · 2016-11-05T15:17:59Z

⚡ Previous build results for arm32, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-wpt1, windows-dev are reusable. Rebuilding only arm64, linux-rel-wpt, mac-rel-css, mac-rel-wpt2...

ducks · 2016-11-05T15:22:29Z

Cool. Yeah I've seen it on other issues but willing to jump in wherever.

SimonSapin · 2016-11-05T20:04:07Z

Trying to get homu unstuck


        remove extra clones from dom event script

KiChjang · 2016-11-05T20:43:40Z

@bors-servo r=frewsxcv

bors-servo · 2016-11-05T20:43:41Z

📌 Commit c5f562a has been approved by frewsxcv

bors-servo · 2016-11-05T20:47:02Z

⌛ Testing commit c5f562a with merge 214e13e...


        Auto merge of #14066 - rjgoldsborough:remove-extra-clones-14062, r=fr…

…ewsxcv remove extra clones from dom event script  fixes #14062 by removing extra clone calls ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14062 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because elements are already copies   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14066)

bors-servo · 2016-11-05T21:15:05Z

⛄ The build was interrupted to prioritize another pull request.

KiChjang · 2016-11-06T02:25:03Z

@bors-servo retry clean

bors-servo · 2016-11-06T02:55:34Z

⌛ Testing commit c5f562a with merge bc9a8f5...


        Auto merge of #14066 - rjgoldsborough:remove-extra-clones-14062, r=fr…

…ewsxcv remove extra clones from dom event script  fixes #14062 by removing extra clone calls ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14062 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because elements are already copies   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14066)

bors-servo · 2016-11-06T03:57:42Z

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

highfive assigned nox Nov 4, 2016

highfive added the S-awaiting-review label Nov 4, 2016

nox reviewed Nov 4, 2016

View changes

nox added S-needs-code-changes and removed S-awaiting-review labels Nov 4, 2016

ducks force-pushed the ducks:remove-extra-clones-14062 branch from 1a0d903 to 20cf7c1 Nov 4, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 4, 2016

ducks force-pushed the ducks:remove-extra-clones-14062 branch from 20cf7c1 to 5b22f85 Nov 4, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 4, 2016

highfive assigned frewsxcv and unassigned nox Nov 4, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 5, 2016

highfive added the S-tests-failed label Nov 5, 2016

SimonSapin closed this Nov 5, 2016

SimonSapin reopened this Nov 5, 2016

SimonSapin closed this Nov 5, 2016

SimonSapin reopened this Nov 5, 2016

remove extra clones from dom event script

Loading status checks…

c5f562a

KiChjang force-pushed the ducks:remove-extra-clones-14062 branch from 5b22f85 to c5f562a Nov 5, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Nov 5, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 5, 2016

bors-servo merged commit c5f562a into servo:master Nov 6, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

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

servo / servo

remove extra clones from dom event script #14066

remove extra clones from dom event script #14066

ducks commented Nov 4, 2016 •

edited by larsbergstrom

highfive commented Nov 4, 2016

highfive commented Nov 4, 2016

nox left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ducks commented Nov 4, 2016 •

edited

ducks commented Nov 4, 2016

frewsxcv commented Nov 4, 2016 •

edited

ducks commented Nov 4, 2016

frewsxcv commented Nov 4, 2016

bors-servo commented Nov 4, 2016

frewsxcv commented Nov 4, 2016

ducks commented Nov 4, 2016

bors-servo commented Nov 4, 2016

bors-servo commented Nov 5, 2016

highfive commented Nov 5, 2016

nox commented Nov 5, 2016

highfive commented Nov 5, 2016

frewsxcv commented Nov 5, 2016

ducks commented Nov 5, 2016

frewsxcv commented Nov 5, 2016

bors-servo commented Nov 5, 2016

ducks commented Nov 5, 2016

SimonSapin commented Nov 5, 2016

KiChjang commented Nov 5, 2016

bors-servo commented Nov 5, 2016

bors-servo commented Nov 5, 2016

bors-servo commented Nov 5, 2016

KiChjang commented Nov 6, 2016

bors-servo commented Nov 6, 2016

bors-servo commented Nov 6, 2016

servo / servo

Join GitHub today

remove extra clones from dom event script #14066

remove extra clones from dom event script #14066

Conversation

ducks commented Nov 4, 2016 • edited by larsbergstrom

highfive commented Nov 4, 2016

highfive commented Nov 4, 2016

nox left a comment

This comment has been minimized.

nox Nov 4, 2016

This comment has been minimized.

frewsxcv Nov 4, 2016

This comment has been minimized.

frewsxcv Nov 4, 2016

ducks commented Nov 4, 2016 • edited

ducks commented Nov 4, 2016

frewsxcv commented Nov 4, 2016 • edited

ducks commented Nov 4, 2016

frewsxcv commented Nov 4, 2016

bors-servo commented Nov 4, 2016

frewsxcv commented Nov 4, 2016

ducks commented Nov 4, 2016

bors-servo commented Nov 4, 2016

bors-servo commented Nov 5, 2016

highfive commented Nov 5, 2016

nox commented Nov 5, 2016

highfive commented Nov 5, 2016

frewsxcv commented Nov 5, 2016

ducks commented Nov 5, 2016

frewsxcv commented Nov 5, 2016

bors-servo commented Nov 5, 2016

ducks commented Nov 5, 2016

SimonSapin commented Nov 5, 2016

KiChjang commented Nov 5, 2016

bors-servo commented Nov 5, 2016

bors-servo commented Nov 5, 2016

bors-servo commented Nov 5, 2016

KiChjang commented Nov 6, 2016

bors-servo commented Nov 6, 2016

bors-servo commented Nov 6, 2016

ducks commented Nov 4, 2016 •

edited by larsbergstrom

ducks commented Nov 4, 2016 •

edited

frewsxcv commented Nov 4, 2016 •

edited