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

Consider precomputing Element#tagName. #1471

Closed
zmike opened this issue Jan 8, 2014 · 7 comments · Fixed by #11318
Closed

Consider precomputing Element#tagName. #1471

zmike opened this issue Jan 8, 2014 · 7 comments · Fixed by #11318
Labels
A-content/dom Interacting with the DOM from web content C-assigned There is someone working on resolving the issue C-has-open-pr There is a PR open that resolves the issue E-less-complex Straightforward. Recommended for a new contributor. I-perf-slow Unnecessary performance degredation.

Comments

@zmike
Copy link
Contributor

zmike commented Jan 8, 2014

According to jgraham:

It looks like someone didn't finish the namespace implementation for elements
At the moment they store something called tag_name
But they should store local_name and tag_name should be computed

@bzbarsky
Copy link
Contributor

bzbarsky commented Jan 8, 2014

Computing tag_name is slow, actually. We want to be storing both, probably in something like Gecko's shared nodeinfo structs.

@jdm
Copy link
Member

jdm commented Jan 8, 2014

#854 is relevant, in that case.

@jdm
Copy link
Member

jdm commented Oct 21, 2014

Pretty sure Node.name and Node.local_name cover this.

@jdm jdm closed this as completed Oct 21, 2014
@jdm jdm reopened this Oct 21, 2014
@jdm
Copy link
Member

jdm commented Oct 21, 2014

I can't read. We now store Element.local_name and compute the tag name.

@Ms2ger Ms2ger changed the title dom::element does not have local_name struct member Consider precomputing Element#tagName. Oct 21, 2014
@mbrubeck mbrubeck added A-content/dom Interacting with the DOM from web content I-perf-slow Unnecessary performance degredation. E-less-complex Straightforward. Recommended for a new contributor. labels May 20, 2016
@mbrubeck
Copy link
Contributor

If this is still something we want to fix, it would involve taking the logic from Element::TagName and performing it when the element is created (for example in Element::new_inherited_with_state) and storing the result in a new field of the Element struct. Then TagName could just return the value of that field.

@Ms2ger
Copy link
Contributor

Ms2ger commented May 20, 2016

Note that the computed value needs to be updated if the owner document changes.

@bzbarsky
Copy link
Contributor

Right.

I really do think the way Gecko does this (with a shared nodeinfo struct) is pretty good, I don't know to what extent the sharing would be a problem for Servo...

bors-servo pushed a commit that referenced this issue May 21, 2016
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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11318)
<!-- Reviewable:end -->
@jdm jdm added the C-assigned There is someone working on resolving the issue label May 24, 2016
@jdm jdm added the C-has-open-pr There is a PR open that resolves the issue label Jun 8, 2016
bors-servo pushed a commit that referenced this issue Jul 20, 2016
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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11318)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Aug 4, 2016
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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11318)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/dom Interacting with the DOM from web content C-assigned There is someone working on resolving the issue C-has-open-pr There is a PR open that resolves the issue E-less-complex Straightforward. Recommended for a new contributor. I-perf-slow Unnecessary performance degredation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants