Compute tag_name a maximum of once per document owner #11318

mitchhentges · 2016-05-21T14:58:10Z

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

./mach build -d does not report any errors
./mach test-tidy --faster does not report any errors
These changes fix Consider precomputing Element#tagName. #1471 (github issue number if applicable).

Either:

These changes do not require tests because no new functionality was added, just a reorganization and caching of existing functionality

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

This change is

highfive · 2016-05-21T14:58:14Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

highfive · 2016-05-21T14:58:15Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/element.rs

highfive · 2016-05-21T14:58:15Z

Warning

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

nox · 2016-05-21T15:51:02Z

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

Nice work, @mitchhentges.

Previously, highfive wrote…

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/element.rs

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

components/script/dom/element.rs, line 68 [r1] (raw file):

use dom::validation::Validatable;
use dom::virtualmethods::{VirtualMethods, vtable_for};
use heapsize::{HeapSizeOf};

Nit: remove braces.

components/script/dom/element.rs, line 1305 [r1] (raw file):

                qualified_name.to_ascii_uppercase()
            } else {
                qualified_name.into_owned()

There is no need for into_owned here now that there is an implementation of From<Cow<str>> for Atom.

components/script/dom/element.rs, line 2104 [r1] (raw file):

    fn adopting_steps(&self, old_doc: &Document) {
        self.super_type().unwrap().adopting_steps(old_doc);
        self.tag_name.clear();

Just noticed that you can limit the clear call to cases such:

document_from_node(self).is_html_document() != old_doc.is_html_document()

components/script/dom/element.rs, line 2552 [r1] (raw file):

// A holder for an element's "tag name", which will be lazily
// resolved and cached. Should be reset when the document
// owner change

Nit: three slashes and a final period.

components/script/dom/element.rs, line 2566 [r1] (raw file):

impl TagName {
    pub fn new() -> TagName {

Make it private.

components/script/dom/element.rs, line 2573 [r1] (raw file):

    /// initialized with the result of `cb` first.
    #[allow(unsafe_code)]
    pub fn or_init<F>(&self, cb: F) -> Atom

Make it private.

components/script/dom/element.rs, line 2587 [r1] (raw file):

    // Clear the cached tag name, so that it will be re-calculated the
    // next time that `or_init()` is called

Nit: period.

components/script/dom/element.rs, line 2589 [r1] (raw file):

    // next time that `or_init()` is called
    #[allow(unsafe_code)]
    pub fn clear(&self) {

Make it private.

components/script/dom/element.rs, line 2592 [r1] (raw file):

        unsafe {
            let ptr = self.ptr.get();
            *ptr = None;

Nit: no need for the intermediate ptr variable.

components/script/dom/element.rs, line 2595 [r1] (raw file):

        }
    }
}

Nit: final empty line.

Comments from Reviewable

highfive · 2016-05-21T16:16:00Z

New code was committed to pull request.

highfive · 2016-05-21T16:17:38Z

New code was committed to pull request.

highfive · 2016-05-21T16:18:03Z

New code was committed to pull request.

highfive · 2016-05-21T16:20:31Z

New code was committed to pull request.

nox · 2016-05-21T16:24:45Z

@bors-servo try

-S-awaiting-review

@Ms2ger Do you know if there are tests for changing a element's owner document and checking its tag name?

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bors-servo · 2016-05-21T16:24:50Z

⌛ Trying commit d3a987f with merge 78df8d3...

Compute tag_name a maximum of once per document owner Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy --faster` does not report any errors - [X] These changes fix #1471 (github issue number if applicable). Either: - [X] These changes do not require tests because no new functionality was added, just a reorganization and caching of existing functionality Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11318)

bors-servo · 2016-05-21T16:38:20Z

💔 Test failed - linux-dev

KiChjang · 2016-05-21T16:52:00Z

Unit tests are failing:

failures:
---- size_of::size_element stdout ----
    thread 'size_of::size_element' panicked at 'Your changes have increased the stack size of commonly used DOM struct Element from 312 to 328. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in tests/unit/script/size_of.rs.', /home/travis/build/servo/servo/tests/unit/script/size_of.rs:42
stack backtrace:
   1:     0x7f0faf558190 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
   2:     0x7f0faf55babb - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
   3:     0x7f0faf55b6ac - std::panicking::default_hook::hc2c969e7453d080c
   4:     0x7f0faf54f138 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
   5:     0x7f0faf55bd01 - std::panicking::begin_panic::h4889569716505182
   6:     0x7f0faf5500ba - std::panicking::begin_panic_fmt::h484cd47786497f03
   7:     0x7f0faed3e3c1 - script_tests::size_of::size_element::he2aeb340c66061cc
                        at /home/travis/build/servo/servo/components/servo/<std macros>:8
   8:     0x7f0faf533906 - _<F as std..boxed..FnBox<A>>::call_box::hebdedc6b66e381cc
   9:     0x7f0faf536037 - std::panicking::try::call::hdb2c8d43be906104
  10:     0x7f0faf56593b - __rust_try
  11:     0x7f0faf5658de - __rust_maybe_catch_panic
  12:     0x7f0faf53645e - _<F as std..boxed..FnBox<A>>::call_box::h214ad3593915cb36
  13:     0x7f0faf55a0d4 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  14:     0x7f0facdaf181 - start_thread
  15:     0x7f0fac8c647c - __clone
  16:                0x0 - <unknown>
---- size_of::size_div stdout ----
    thread 'size_of::size_div' panicked at 'Your changes have increased the stack size of commonly used DOM struct HTMLDivElement from 328 to 344. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in tests/unit/script/size_of.rs.', /home/travis/build/servo/servo/tests/unit/script/size_of.rs:44
stack backtrace:
   1:     0x7f0faf558190 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
   2:     0x7f0faf55babb - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
   3:     0x7f0faf55b6ac - std::panicking::default_hook::hc2c969e7453d080c
   4:     0x7f0faf54f138 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
   5:     0x7f0faf55bd01 - std::panicking::begin_panic::h4889569716505182
   6:     0x7f0faf5500ba - std::panicking::begin_panic_fmt::h484cd47786497f03
   7:     0x7f0faed3ea41 - script_tests::size_of::size_div::hca661af76121f93c
                        at /home/travis/build/servo/servo/components/servo/<std macros>:8
   8:     0x7f0faf533906 - _<F as std..boxed..FnBox<A>>::call_box::hebdedc6b66e381cc
   9:     0x7f0faf536037 - std::panicking::try::call::hdb2c8d43be906104
  10:     0x7f0faf56593b - __rust_try
  11:     0x7f0faf5658de - __rust_maybe_catch_panic
  12:     0x7f0faf53645e - _<F as std..boxed..FnBox<A>>::call_box::h214ad3593915cb36
  13:     0x7f0faf55a0d4 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  14:     0x7f0facdaf181 - start_thread
  15:     0x7f0fac8c647c - __clone
  16:                0x0 - <unknown>
---- size_of::size_span stdout ----
    thread 'size_of::size_span' panicked at 'Your changes have increased the stack size of commonly used DOM struct HTMLSpanElement from 328 to 344. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in tests/unit/script/size_of.rs.', /home/travis/build/servo/servo/tests/unit/script/size_of.rs:45
stack backtrace:
   1:     0x7f0faf558190 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
   2:     0x7f0faf55babb - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
   3:     0x7f0faf55b6ac - std::panicking::default_hook::hc2c969e7453d080c
   4:     0x7f0faf54f138 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
   5:     0x7f0faf55bd01 - std::panicking::begin_panic::h4889569716505182
   6:     0x7f0faf5500ba - std::panicking::begin_panic_fmt::h484cd47786497f03
   7:     0x7f0faed3ed81 - script_tests::size_of::size_span::h01b04b425602691c
                        at /home/travis/build/servo/servo/components/servo/<std macros>:8
   8:     0x7f0faf533906 - _<F as std..boxed..FnBox<A>>::call_box::hebdedc6b66e381cc
   9:     0x7f0faf536037 - std::panicking::try::call::hdb2c8d43be906104
  10:     0x7f0faf56593b - __rust_try
  11:     0x7f0faf5658de - __rust_maybe_catch_panic
  12:     0x7f0faf53645e - _<F as std..boxed..FnBox<A>>::call_box::h214ad3593915cb36
  13:     0x7f0faf55a0d4 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  14:     0x7f0facdaf181 - start_thread
  15:     0x7f0fac8c647c - __clone
  16:                0x0 - <unknown>
---- size_of::size_htmlelement stdout ----
    thread 'size_of::size_htmlelement' panicked at 'Your changes have increased the stack size of commonly used DOM struct HTMLElement from 328 to 344. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in tests/unit/script/size_of.rs.', /home/travis/build/servo/servo/tests/unit/script/size_of.rs:43
stack backtrace:
   1:     0x7f0faf558190 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
   2:     0x7f0faf55babb - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
   3:     0x7f0faf55b6ac - std::panicking::default_hook::hc2c969e7453d080c
   4:     0x7f0faf54f138 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
   5:     0x7f0faf55bd01 - std::panicking::begin_panic::h4889569716505182
   6:     0x7f0faf5500ba - std::panicking::begin_panic_fmt::h484cd47786497f03
   7:     0x7f0faed3e701 - script_tests::size_of::size_htmlelement::h3acca56fab8b1b38
                        at /home/travis/build/servo/servo/components/servo/<std macros>:8
   8:     0x7f0faf533906 - _<F as std..boxed..FnBox<A>>::call_box::hebdedc6b66e381cc
   9:     0x7f0faf536037 - std::panicking::try::call::hdb2c8d43be906104
  10:     0x7f0faf56593b - __rust_try
  11:     0x7f0faf5658de - __rust_maybe_catch_panic
  12:     0x7f0faf53645e - _<F as std..boxed..FnBox<A>>::call_box::h214ad3593915cb36
  13:     0x7f0faf55a0d4 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  14:     0x7f0facdaf181 - start_thread
  15:     0x7f0fac8c647c - __clone
  16:                0x0 - <unknown>

Ms2ger · 2016-05-21T22:45:36Z

I'm sorry, but this is r- from me because of the unsafe code.

KiChjang · 2016-05-21T22:54:44Z

components/script/dom/element.rs

+/// owner changes.
+#[derive(JSTraceable)]
+struct TagName {
+    ptr: UnsafeCell<Option<Atom>>,


I agree with @Ms2ger, why is this an UnsafeCell rather than a Cell or RefCell?

nox · 2016-05-21T23:42:14Z

@Ms2ger It's exactly the same scheme as UniqueId in node.rs, I'm the one who mentioned UnsafeCell to not waste space for something that is just lazily-initiated. We could just use RefCell, but I would rather not make all elements 16 bytes larger.

@KiChjang We can't use Cell, Atom is not Copy.

nox · 2016-07-20T09:26:05Z

@bors-servo try

I think this needs expectation changes, so try instead of r+.

bors-servo · 2016-07-20T09:26:08Z

⌛ Trying commit cc67a80 with merge 3bd2b54...

Compute tag_name a maximum of once per document owner Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: - [x] `./mach build -d` does not report any errors - [X] `./mach test-tidy --faster` does not report any errors - [X] These changes fix #1471 (github issue number if applicable). Either: - [X] These changes do not require tests because no new functionality was added, just a reorganization and caching of existing functionality Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11318)

bors-servo · 2016-07-20T10:24:31Z

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

mitchhentges · 2016-07-20T15:16:54Z

Just these four expectation changes, right?

nox · 2016-08-01T14:26:27Z

No, we didn't implement the right thing before, so there should be new unexpected passing tests. The fact that there are none means new tests need to be written.

Ms2ger · 2016-08-01T15:09:18Z

Hold on, did this change behaviour?

nox · 2016-08-01T15:11:42Z

Mmmh guess I was wrong about that, sorry it has been a long time since I looked at that PR. :/

Ms2ger · 2016-08-01T15:14:51Z

components/script/dom/element.rs

+
+    /// Retrieve a copy of the current inner value. If it is `None`, it is
+    /// initialized with the result of `cb` first.
+    #[allow(unsafe_code)]


Let's remove the two #[allow(unsafe_code)] attributes; should then be good to go.

mitchhentges · 2016-08-04T03:05:56Z

18th time the charm 😉

nox · 2016-08-04T07:16:34Z

@bors-servo r+

Thanks again for your contribution!

bors-servo · 2016-08-04T07:16:35Z

📌 Commit 1ca4a3e has been approved by nox

bors-servo · 2016-08-04T07:27:21Z

⌛ Testing commit 1ca4a3e with merge 1837fcb...

Compute tag_name a maximum of once per document owner Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: - [x] `./mach build -d` does not report any errors - [X] `./mach test-tidy --faster` does not report any errors - [X] These changes fix #1471 (github issue number if applicable). Either: - [X] These changes do not require tests because no new functionality was added, just a reorganization and caching of existing functionality Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11318)

bors-servo · 2016-08-04T08:30:08Z

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

highfive assigned Manishearth May 21, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 21, 2016

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 May 21, 2016

nox assigned nox and unassigned Manishearth May 21, 2016

mitchhentges force-pushed the 1471-cache-tag-name branch from d63d07a to 4760443 Compare May 21, 2016 16:15

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 May 21, 2016

mitchhentges force-pushed the 1471-cache-tag-name branch from 4760443 to 67fb968 Compare May 21, 2016 16:17

mitchhentges force-pushed the 1471-cache-tag-name branch from 67fb968 to 93ac2c8 Compare May 21, 2016 16:18

mitchhentges force-pushed the 1471-cache-tag-name branch from 93ac2c8 to d3a987f Compare May 21, 2016 16:20

nox removed the S-awaiting-review There is new code that needs to be reviewed. label May 21, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label May 21, 2016

KiChjang reviewed May 21, 2016
View reviewed changes

jdm removed the S-needs-rebase There are merge conflict errors. label Jul 19, 2016

mitchhentges force-pushed the 1471-cache-tag-name branch from 84a8cd8 to cc67a80 Compare July 19, 2016 15:18

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jul 19, 2016

nox added S-needs-tests New tests have been requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 1, 2016

nox removed the S-needs-tests New tests have been requested by a reviewer. label Aug 1, 2016

Ms2ger reviewed Aug 1, 2016
View reviewed changes

jdm added the S-needs-code-changes Changes have not yet been made that were requested by a reviewer. label Aug 1, 2016

Compute tag_name a maximum of once per document owner

1ca4a3e

mitchhentges force-pushed the 1471-cache-tag-name branch from cc67a80 to 1ca4a3e Compare August 4, 2016 03:05

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 Aug 4, 2016

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 Aug 4, 2016

bors-servo merged commit 1ca4a3e into servo:master Aug 4, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute tag_name a maximum of once per document owner #11318

Compute tag_name a maximum of once per document owner #11318

mitchhentges commented May 21, 2016 •

edited

highfive commented May 21, 2016

highfive commented May 21, 2016

highfive commented May 21, 2016

nox commented May 21, 2016

highfive commented May 21, 2016

highfive commented May 21, 2016

highfive commented May 21, 2016

highfive commented May 21, 2016

nox commented May 21, 2016

bors-servo commented May 21, 2016

bors-servo commented May 21, 2016

KiChjang commented May 21, 2016

Ms2ger commented May 21, 2016

KiChjang May 21, 2016

nox commented May 21, 2016

nox commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 20, 2016

mitchhentges commented Jul 20, 2016

nox commented Aug 1, 2016

Ms2ger commented Aug 1, 2016

nox commented Aug 1, 2016

Ms2ger Aug 1, 2016

mitchhentges commented Aug 4, 2016

nox commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 4, 2016

Compute tag_name a maximum of once per document owner #11318

Compute tag_name a maximum of once per document owner #11318

Conversation

mitchhentges commented May 21, 2016 • edited

highfive commented May 21, 2016

highfive commented May 21, 2016

highfive commented May 21, 2016

nox commented May 21, 2016

highfive commented May 21, 2016

highfive commented May 21, 2016

highfive commented May 21, 2016

highfive commented May 21, 2016

nox commented May 21, 2016

bors-servo commented May 21, 2016

bors-servo commented May 21, 2016

KiChjang commented May 21, 2016

Ms2ger commented May 21, 2016

KiChjang May 21, 2016

Choose a reason for hiding this comment

nox commented May 21, 2016

nox commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 20, 2016

mitchhentges commented Jul 20, 2016

nox commented Aug 1, 2016

Ms2ger commented Aug 1, 2016

nox commented Aug 1, 2016

Ms2ger Aug 1, 2016

Choose a reason for hiding this comment

mitchhentges commented Aug 4, 2016

nox commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 4, 2016

bors-servo commented Aug 4, 2016

mitchhentges commented May 21, 2016 •

edited