(no longer active) Assignee
	Description • 7 years ago

This is similar to bug 1342720 but more generally...

	(no longer active) 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.

	David Baron :dbaron:
	Comment 3 • 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.)

	Jonathan Kew [: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.

	Jonathan Kew [:jfkthame]
	Comment 5 • 7 years ago

ni? to Ehsan to notice the above comment, as I didn't actually complete a review here yet...

	(no longer active) 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.  :-)

	(no longer active) 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.

	(no longer active) 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...)

	Jonathan Kew [:jfkthame]
	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.

	Jonathan Kew [:jfkthame]
	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.)

	(no longer active) 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.

	Pulsebot
	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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 14 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a8e6a2c7e926
https://hg.mozilla.org/mozilla-central/rev/ea2dc5a6091d