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

Style needs a way to get font metrics while computing style data. #14079

Open
emilio opened this issue Nov 5, 2016 · 10 comments
Open

Style needs a way to get font metrics while computing style data. #14079

emilio opened this issue Nov 5, 2016 · 10 comments

Comments

@emilio
Copy link
Member

@emilio emilio commented Nov 5, 2016

This right now isn't implemented (see #7462), and I'm afraid it might be a pain to do for Stylo.

Basically, if we find an ex or a ch unit, we need to get the font metrics of the first available font for that style in order to compute the value.

This is not too common, but I think a lot of the font code is lazy in Gecko so calling it from multiple threads can be a very huge pain, and can impose a significant synchronization cost for pages that use this units.

In servo it's also a pain to implement too, since all the font code is on gfx and only layout has access to a FontContext right now.

cc @heycam @bholley @Manishearth @SimonSapin and @upsuper (I think I haven't left anyone :))

@emilio

This comment has been minimized.

Copy link
Member Author

@emilio emilio commented Nov 7, 2016

IRC conversation with @upsuper: http://logs.glob.uno/?c=mozilla%23layout&s=7+Nov+2016&e=7+Nov+2016#c23995

This sounds like a pretty decent amount of work...

@emilio

This comment has been minimized.

Copy link
Member Author

@emilio emilio commented Nov 7, 2016

cc @nox, who has also done font stuff

@bholley

This comment has been minimized.

Copy link
Contributor

@bholley bholley commented Nov 7, 2016

How common are these units? Could we just acquire a mutex, call into Gecko, and cache the result in TLS?

CC @dbaron @pcwalton

@upsuper

This comment has been minimized.

Copy link
Member

@upsuper upsuper commented Nov 7, 2016

Firstly, I think it is probably hard not to do them lazily, because font metrics rely on all kinds of font information, including font family, size, style, variant, etc. It is simply impossible to generate all possible font metrics in advance.

I think the main problem here is that we probably want a global shared metrics cache, and that cache is usually implemented in a fashion that changes the order when accessed. (At least that is the case of Gecko.)

Not sure how common would those units be. I myself hardly use them, but I wouldn't be surprised if some websites want to heavily rely on those units.

@upsuper

This comment has been minimized.

Copy link
Member

@upsuper upsuper commented Nov 7, 2016

An alternative way would be doing restyling in two phases. In the first phases we compute all things which do not rely on dimensions, and we generate a list of font metrics we need. And in the second phase we resolve all dimension-related properties.

@pcwalton

This comment has been minimized.

Copy link
Contributor

@pcwalton pcwalton commented Nov 8, 2016

How about a reader-writer lock cache? I think a RW lock may be fast enough since these units are not that common.

An alternative would be per thread caches to eliminate the synchronization cost.

@bholley

This comment has been minimized.

Copy link
Contributor

@bholley bholley commented Nov 8, 2016

Yeah, RWLock might be fine too, especially with parking lot. Roughly isomorphic to a TLS cache + mutex from an architectural perspective, so I think we should try these approaches before doing anything much more complicated.

emilio added a commit to emilio/servo that referenced this issue Nov 11, 2016
This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle servo#14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.
emilio added a commit to emilio/servo that referenced this issue Nov 11, 2016
This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle servo#14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.
emilio added a commit to emilio/servo that referenced this issue Nov 11, 2016
This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle servo#14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.
bors-servo added a commit that referenced this issue Nov 11, 2016
style: Refactor and add infrastructure for font metrics in style.

<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because moves stuff around without adding functionality.

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

This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle #14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.

<!-- 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/14174)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue Nov 11, 2016
This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle servo#14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.
bors-servo added a commit that referenced this issue Nov 11, 2016
style: Refactor and add infrastructure for font metrics in style.

<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because moves stuff around without adding functionality.

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

This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle #14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.

<!-- 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/14174)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 11, 2016
style: Refactor and add infrastructure for font metrics in style.

<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because moves stuff around without adding functionality.

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

This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle #14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.

<!-- 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/14174)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 11, 2016
style: Refactor and add infrastructure for font metrics in style.

<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because moves stuff around without adding functionality.

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

This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle #14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.

<!-- 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/14174)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue Nov 11, 2016
This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle servo#14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.
bors-servo added a commit that referenced this issue Nov 11, 2016
style: Refactor and add infrastructure for font metrics in style.

<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because moves stuff around without adding functionality.

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

This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle #14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.

<!-- 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/14174)
<!-- Reviewable:end -->
emilio added a commit to emilio/servo that referenced this issue Nov 13, 2016
This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle servo#14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.
bors-servo added a commit that referenced this issue Nov 13, 2016
style: Refactor and add infrastructure for font metrics in style.

<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because moves stuff around without adding functionality.

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

This commit itself only moves things around and adds an extra parameter to the
`apply_declarations` function to eventually handle #14079 correctly.

Probably needs a more granular API to query fonts, á la nsFontMetrics, but
that's trivial to do once this is landed.

Then we should make the font provider mandatory, and implement the missing stylo
bits.

<!-- 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/14174)
<!-- Reviewable:end -->
@emilio

This comment has been minimized.

Copy link
Member Author

@emilio emilio commented Nov 18, 2016

I added infra for this in #14174, though it still needs a bunch of work of moving code from gfx into style, or into another crate lower in the dependency chain.

@nox

This comment has been minimized.

Copy link
Member

@nox nox commented Mar 4, 2017

@emilio Is this still true?

@emilio

This comment has been minimized.

Copy link
Member Author

@emilio emilio commented Mar 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.