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.

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

HashMap (and therefore DOM nodes) grew by 8 bytes #15704

Closed
SimonSapin opened this issue Feb 23, 2017 · 4 comments
Closed

HashMap (and therefore DOM nodes) grew by 8 bytes #15704

SimonSapin opened this issue Feb 23, 2017 · 4 comments

Comments

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Feb 23, 2017

HashMap grew by 8 bytes in rust-lang/rust#38368. script::dom::event_target::EventTarget contains:

    handlers: DOMRefCell<HashMap<Atom, EventListeners, BuildHasherDefault<FnvHasher>>>,

and DOM nodes contain EventTarget, so they all grew by the same amount, and unit tests in tests/unit/script/size_of.rs fail when updating the compiler: #15565 (comment)

@jdm @Ms2ger @nox What do you think? Should we take the hit and increase the expect size in unit tests, or try to get this space back somehow?

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Feb 23, 2017

Is the number of handlers for a given event target expected to be small? Could we get away with a Vec and linear scan instead of a HashMap?

I’m also looking at BTreeMap which is smaller (24 bytes), but in the current implementation even an empty map allocates an internal LeafNode with space for 11 key/value pairs, which in this case is 352 bytes. (For comparison, HashMap grew from 40 to 48 bytes but an empty map does not allocate.)

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Feb 23, 2017

Correction: 40/48 is with the default RandomState which takes 16 bytes. BuildHasherDefault is zero bytes, so the HashMap in handlers grew from 24 to 32 bytes.

@nox
Copy link
Member

@nox nox commented Feb 23, 2017

I don't care either way. One day we will optimise such things extensively, but today is not the day.

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Feb 23, 2017

Ok. Let’s increase expected sizes in unit tests next time we rustup.

SimonSapin added a commit that referenced this issue Feb 23, 2017
nox added a commit to nox/servo that referenced this issue Feb 24, 2017
SimonSapin added a commit that referenced this issue Mar 11, 2017
This fixes the DOM node size regression introduced by a previous Rust update:
#15704
bors-servo added a commit that referenced this issue Mar 11, 2017
Update to rustc 1.17.0-nightly (8c72b7651 2017-03-11)

This fixes the DOM node size regression introduced by a previous Rust update: #15704

<!-- Please describe your changes on the following line: -->

---
<!-- 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` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

<!-- 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/15911)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Mar 11, 2017
Update to rustc 1.17.0-nightly (8c72b7651 2017-03-11)

This fixes the DOM node size regression introduced by a previous Rust update: #15704

<!-- Please describe your changes on the following line: -->

---
<!-- 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` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

<!-- 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/15911)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 13, 2017
…03-11) (from servo:rustup_); r=emilio

This fixes the DOM node size regression introduced by a previous Rust update: servo/servo#15704

<!-- Please describe your changes on the following line: -->

---
<!-- 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` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

Source-Repo: https://github.com/servo/servo
Source-Revision: e102577fe498c8f0cee43ba80b21dc9430be1e2b

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 034771746b27e5a6c7e2e96948a366d26f92cd6b
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this issue Mar 15, 2017
…03-11) (from servo:rustup_); r=emilio

This fixes the DOM node size regression introduced by a previous Rust update: servo/servo#15704

<!-- Please describe your changes on the following line: -->

---
<!-- 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` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

Source-Repo: https://github.com/servo/servo
Source-Revision: e102577fe498c8f0cee43ba80b21dc9430be1e2b
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this issue Mar 16, 2017
…03-11) (from servo:rustup_); r=emilio

This fixes the DOM node size regression introduced by a previous Rust update: servo/servo#15704

<!-- Please describe your changes on the following line: -->

---
<!-- 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` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

Source-Repo: https://github.com/servo/servo
Source-Revision: e102577fe498c8f0cee43ba80b21dc9430be1e2b
clementmiao added a commit to clementmiao/servo that referenced this issue Apr 7, 2017
clementmiao added a commit to clementmiao/servo that referenced this issue Apr 7, 2017
This fixes the DOM node size regression introduced by a previous Rust update:
servo#15704
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
…03-11) (from servo:rustup_); r=emilio

This fixes the DOM node size regression introduced by a previous Rust update: servo/servo#15704

<!-- Please describe your changes on the following line: -->

---
<!-- 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` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

Source-Repo: https://github.com/servo/servo
Source-Revision: e102577fe498c8f0cee43ba80b21dc9430be1e2b

UltraBlame original commit: e6e23086b53b57b3b3b4da8fd99d6bcb9159a317
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
…03-11) (from servo:rustup_); r=emilio

This fixes the DOM node size regression introduced by a previous Rust update: servo/servo#15704

<!-- Please describe your changes on the following line: -->

---
<!-- 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` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

Source-Repo: https://github.com/servo/servo
Source-Revision: e102577fe498c8f0cee43ba80b21dc9430be1e2b

UltraBlame original commit: e6e23086b53b57b3b3b4da8fd99d6bcb9159a317
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…03-11) (from servo:rustup_); r=emilio

This fixes the DOM node size regression introduced by a previous Rust update: servo/servo#15704

<!-- Please describe your changes on the following line: -->

---
<!-- 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` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

Source-Repo: https://github.com/servo/servo
Source-Revision: e102577fe498c8f0cee43ba80b21dc9430be1e2b

UltraBlame original commit: e6e23086b53b57b3b3b4da8fd99d6bcb9159a317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.