Closed
Bug 1352527
Opened 7 years ago
Closed 7 years ago
Put BidiPresUtils on a malloc/free diet
Categories
(Core :: Layout, enhancement)
Core
Layout
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(2 files, 3 obsolete files)
3.69 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
5.12 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
This is similar to bug 1342720 but more generally...
Assignee | ||
Comment 1•7 years ago
|
||
The nsBidi API requires the consumer to first call SetPara() in order to perform bidi resolution and that resets the data members, so there is no need to recreate nsBidi objects from scratch each time we need to perform bidi resolution. Having this API allows us to reuse this object across the calls to nsBidiPresUtils members.
Attachment #8853712 -
Flags: review?(dbaron)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8853713 -
Flags: review?(dbaron)
Assignee | ||
Updated•7 years ago
|
Blocks: gdoc_pageend
Assignee | ||
Updated•7 years ago
|
Comment on attachment 8853712 [details] [diff] [review] Part 1: Add the nsLayoutUtils::GetBidiEngine() API Should have realized this faster, but Jonathan is a better reviewer for this. (I wonder if there are assertions that would be useful to add, though.)
Attachment #8853712 -
Flags: review?(dbaron) → review?(jfkthame)
Attachment #8853713 -
Flags: review?(dbaron) → review?(jfkthame)
Comment 4•7 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #3) > (I wonder if there are assertions that would be useful to add, though.) As :dbaron suggests, I think this could use some assertions -- specifically, to confirm that the static nsBidi engine returned by nsLayoutUtils is only used by one "consumer" at a time. We'd require that the nsBidiPresUtils being converted here can never recurse, or be used from multiple threads. Currently, I think that's be true, but it's not obvious that it will always be the case. A slight variation of this: what would you think of embedding an nsBidi engine in the presContext, rather than making it a static in nsLayoutUtils? That way each presContext would have its own instance, which might make it easier to reason/assert about lifetime and usage, and presumably we don't create/destroy presContexts so frequently that there'd be any significant perf difference from the single app-lifetime instance in layoutUtils.
Comment 5•7 years ago
|
||
ni? to Ehsan to notice the above comment, as I didn't actually complete a review here yet...
Flags: needinfo?(ehsan)
Updated•7 years ago
|
Whiteboard: [qf]
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #4) > (In reply to David Baron :dbaron: ⌚️UTC-7 from comment #3) > > (I wonder if there are assertions that would be useful to add, though.) > > As :dbaron suggests, I think this could use some assertions -- specifically, > to confirm that the static nsBidi engine returned by nsLayoutUtils is only > used by one "consumer" at a time. We'd require that the nsBidiPresUtils > being converted here can never recurse, or be used from multiple threads. > Currently, I think that's be true, but it's not obvious that it will always > be the case. What kind of assertion specifically? nsBidi's code isn't re-enterant in any way that I can see. > A slight variation of this: what would you think of embedding an nsBidi > engine in the presContext, rather than making it a static in nsLayoutUtils? > That way each presContext would have its own instance, which might make it > easier to reason/assert about lifetime and usage, and presumably we don't > create/destroy presContexts so frequently that there'd be any significant > perf difference from the single app-lifetime instance in layoutUtils. Actually now that you mentioned it there is a good reason to do this, we want to be able to interrupt reflows some day to run tasks from other tab group. I guess part of my brain is still operating in the old world. :-)
Flags: needinfo?(ehsan) → needinfo?(jfkthame)
Assignee | ||
Comment 7•7 years ago
|
||
The nsBidi API requires the consumer to first call SetPara() in order to perform bidi resolution and that resets the data members, so there is no need to recreate nsBidi objects from scratch each time we need to perform bidi resolution. Having this API allows us to reuse this object across the calls to nsBidiPresUtils members.
Attachment #8856809 -
Flags: review?(jfkthame)
Assignee | ||
Updated•7 years ago
|
Attachment #8853712 -
Attachment is obsolete: true
Attachment #8853713 -
Attachment is obsolete: true
Attachment #8853712 -
Flags: review?(jfkthame)
Attachment #8853713 -
Flags: review?(jfkthame)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8856810 -
Flags: review?(dbaron)
Assignee | ||
Comment 9•7 years ago
|
||
(Sorry for the bugspam, the previous patch didn't have the updated commit message, hence the incorrect review request...)
Attachment #8856812 -
Flags: review?(jfkthame)
Assignee | ||
Updated•7 years ago
|
Attachment #8856810 -
Attachment is obsolete: true
Attachment #8856810 -
Flags: review?(dbaron)
Assignee | ||
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p3]
Comment 10•7 years ago
|
||
Comment on attachment 8856809 [details] [diff] [review] Part 1: Add the nsPresContext::GetBidiEngine() API Review of attachment 8856809 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresContext.cpp @@ +3032,5 @@ > +{ > + if (!mBidiEngine) { > + mBidiEngine.reset(new nsBidi()); > + } > + return *mBidiEngine; Along with doing this, please give nsBidi a private, deleted copy-constructor and assignment operator, to avoid the risk that someone assigns the result of GetBidiEngine() to a local (non-reference) variable.
Attachment #8856809 -
Flags: review?(jfkthame) → review+
Comment 11•7 years ago
|
||
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #6) > What kind of assertion specifically? nsBidi's code isn't re-enterant in any > way that I can see. I believe this is main thread-only (right?), but if that ever changed we could be in trouble. So an assertion that GetBidiEngine is only used on the main thread might be a good thing. (I can imagine bidi resolution happening on other threads, e.g. in a worker, but they should -not- be getting the engine from here to do so.)
Flags: needinfo?(jfkthame)
Updated•7 years ago
|
Attachment #8856812 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #11) > (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from > comment #6) > > What kind of assertion specifically? nsBidi's code isn't re-enterant in any > > way that I can see. > > I believe this is main thread-only (right?), but if that ever changed we > could be in trouble. So an assertion that GetBidiEngine is only used on the > main thread might be a good thing. > > (I can imagine bidi resolution happening on other threads, e.g. in a worker, > but they should -not- be getting the engine from here to do so.) Yes, that's a good idea. Will address both comments when landing.
Comment 13•7 years ago
|
||
Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e6a2c7e926 Part 1: Add the nsPresContext::GetBidiEngine() API; r=jfkthame https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2dc5a6091d Part 2: Switch nsBidiPresUtils consumers of nsBidi to use nsLayoutUtils::GetBidiEngine(); r=jfkthame
![]() |
||
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a8e6a2c7e926 https://hg.mozilla.org/mozilla-central/rev/ea2dc5a6091d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•2 years ago
|
Performance Impact: --- → P3
Whiteboard: [qf:p3]
You need to log in
before you can comment on or make changes to this bug.
Description
•