Restrict registered PersistentNodes to non-empty ones.

Restrict registered PersistentNodes to non-empty ones.

It is not uncommon for Persistent<> members to be left as empty/null
upon construction, but possibly written to later during the object's
lifetime. Hence delaying the allocation and registeration of the
Persistent<>'s PersistentNode until it is initialized with a non-null
value avoids the alloc/free overhead for Persistent<>s that are never
initialized. And reduces the number of PersistentNode roots that the
GC has to iterate over.

By adopting that scheme here, the number of PersistentNodes allocated
when loading google.com is (approx) halved. Similar reductions consistently
seen elsewhere.

Dually, we also introduce the deallocation of the PersistentNode upon
updating the Persistent<> with nullptr. This is done so as to allow
a thread to cleanly shut down by first only clearing the persistents it
has allocated (and not require it to somehow separately destruct these
first -- that happens upon clearing.)


R=haraken
BUG=483380
Committed: https://crrev.com/b119d7e4f458aeaf3d53c5e50facac4abf4a2742
git-svn-id: svn://svn.chromium.org/blink/trunk@202197 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[sof, 2015-09-13 12:16:45]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-13 12:16:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1338573003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1338573003/20001

[haraken, 2015-09-13 12:36:55]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-09-13 12:36:56]

I like this change but am a bit concerned about a performance impact, since the
CL is inserting updatePersistentNode() into performance-sensitive methods.

Maybe can we just create a couple of micro-benchmarks and confirm that this
doesn't cause problematic regressions? (Slight regressions are fine.)

https://codereview.chromium.org/1338573003/diff/20001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1338573003/diff/20001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:216: void updatePersistentNode()

Maybe I'd prefer renaming this to assign() and do 'm_raw = ...' in assign().

void assign(T* value) {
  m_raw = value;
  if (m_raw)
    ...;
}

Also you can move checkPointer() into assign().

https://codereview.chromium.org/1338573003/diff/20001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:224: uninitialize();

Will this remove the restriction that a (non-cross-thread) persistent handle
must be destructed before the thread shuts down? After this CL, the thread can
shut down if the persistent handle is cleared (doesn't need to be destructed).

https://codereview.chromium.org/1338573003/diff/20001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:230: if (m_persistentNode || !m_raw)

Is it possible that m_persistentNode is non-null here?

[sof, 2015-09-13 12:47:46]

On 2015/09/13 12:36:56, haraken wrote:
> I like this change but am a bit concerned about a performance impact, since
the
> CL is inserting updatePersistentNode() into performance-sensitive methods.
> 

The updatePersistentNode() part isn't vital, it is mostly in place to handle
some untidy Persistent<> ownership details for one or two objects used across
threads. I can explain further tomorrow.

But, assuming you only clear a Persistent<> at the end of its lifetime, it
doesn't have considerable overhead.

[sof, 2015-09-13 12:50:25]

On 2015/09/13 12:47:46, sof wrote:
> On 2015/09/13 12:36:56, haraken wrote:
> > I like this change but am a bit concerned about a performance impact, since
> the
> > CL is inserting updatePersistentNode() into performance-sensitive methods.
> > 
> 
> The updatePersistentNode() part isn't vital, it is mostly in place to handle
> some untidy Persistent<> ownership details for one or two objects used across
> threads. I can explain further tomorrow.
> 
> But, assuming you only clear a Persistent<> at the end of its lifetime, it
> doesn't have considerable overhead.

That comment is a bit confusingly worded -- updatePersistentNode() uses upon
updating m_raw (not setting it initially) aren't vital.

[haraken, 2015-09-13 13:20:35]

On 2015/09/13 12:50:25, sof wrote:
> On 2015/09/13 12:47:46, sof wrote:
> > On 2015/09/13 12:36:56, haraken wrote:
> > > I like this change but am a bit concerned about a performance impact,
since
> > the
> > > CL is inserting updatePersistentNode() into performance-sensitive methods.
> > > 
> > 
> > The updatePersistentNode() part isn't vital, it is mostly in place to handle
> > some untidy Persistent<> ownership details for one or two objects used
across
> > threads. I can explain further tomorrow.
> > 
> > But, assuming you only clear a Persistent<> at the end of its lifetime, it
> > doesn't have considerable overhead.
> 
> That comment is a bit confusingly worded -- updatePersistentNode() uses upon
> updating m_raw (not setting it initially) aren't vital.

Yes, I agree that it would be rare. What I'm a bit concerned about is the
overhead of the if branches inserted into updatePersistentNode(). (My intuition
is that the overhead wouldn't matter but want to confirm that it actually
doesn't matter.)

[sof, 2015-09-13 13:27:03]

On 2015/09/13 13:20:35, haraken wrote:
> On 2015/09/13 12:50:25, sof wrote:
> > On 2015/09/13 12:47:46, sof wrote:
> > > On 2015/09/13 12:36:56, haraken wrote:
> > > > I like this change but am a bit concerned about a performance impact,
> since
> > > the
> > > > CL is inserting updatePersistentNode() into performance-sensitive
methods.
> > > > 
> > > 
> > > The updatePersistentNode() part isn't vital, it is mostly in place to
handle
> > > some untidy Persistent<> ownership details for one or two objects used
> across
> > > threads. I can explain further tomorrow.
> > > 
> > > But, assuming you only clear a Persistent<> at the end of its lifetime, it
> > > doesn't have considerable overhead.
> > 
> > That comment is a bit confusingly worded -- updatePersistentNode() uses upon
> > updating m_raw (not setting it initially) aren't vital.
> 
> Yes, I agree that it would be rare. What I'm a bit concerned about is the
> overhead of the if branches inserted into updatePersistentNode(). (My
intuition
> is that the overhead wouldn't matter but want to confirm that it actually
> doesn't matter.)

We'll see. But it is worth keeping in mind the amount of alloc/free work
currently done for the PersistentNodes numbers quoted on


https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/6r4ei5OESdc/qjT...

[haraken, 2015-09-13 13:35:15]

On 2015/09/13 13:27:03, sof wrote:
> On 2015/09/13 13:20:35, haraken wrote:
> > On 2015/09/13 12:50:25, sof wrote:
> > > On 2015/09/13 12:47:46, sof wrote:
> > > > On 2015/09/13 12:36:56, haraken wrote:
> > > > > I like this change but am a bit concerned about a performance impact,
> > since
> > > > the
> > > > > CL is inserting updatePersistentNode() into performance-sensitive
> methods.
> > > > > 
> > > > 
> > > > The updatePersistentNode() part isn't vital, it is mostly in place to
> handle
> > > > some untidy Persistent<> ownership details for one or two objects used
> > across
> > > > threads. I can explain further tomorrow.
> > > > 
> > > > But, assuming you only clear a Persistent<> at the end of its lifetime,
it
> > > > doesn't have considerable overhead.
> > > 
> > > That comment is a bit confusingly worded -- updatePersistentNode() uses
upon
> > > updating m_raw (not setting it initially) aren't vital.
> > 
> > Yes, I agree that it would be rare. What I'm a bit concerned about is the
> > overhead of the if branches inserted into updatePersistentNode(). (My
> intuition
> > is that the overhead wouldn't matter but want to confirm that it actually
> > doesn't matter.)
> 
> We'll see. But it is worth keeping in mind the amount of alloc/free work
> currently done for the PersistentNodes numbers quoted on
> 
> 
>
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/6r4ei5OESdc/qjT...

I considered a bit more and agree with you that this CL will improve performance
in theory.

- The if branches will regress performance of operations on non-empty persistent
handles by X%.
- The if branches will improve performance of operations on empty persistent
handles by Y%.
- Y would be larger than X.
- The number of empty persistent handles is larger than the number of non-empty
persistent handles.
- In conclusion, this CL will improve performance overall.

Am I making sense?

[sof, 2015-09-13 14:46:55]

On 2015/09/13 13:35:15, haraken wrote:
> On 2015/09/13 13:27:03, sof wrote:
> > On 2015/09/13 13:20:35, haraken wrote:
> > > On 2015/09/13 12:50:25, sof wrote:
> > > > On 2015/09/13 12:47:46, sof wrote:
> > > > > On 2015/09/13 12:36:56, haraken wrote:
> > > > > > I like this change but am a bit concerned about a performance
impact,
> > > since
> > > > > the
> > > > > > CL is inserting updatePersistentNode() into performance-sensitive
> > methods.
> > > > > > 
> > > > > 
> > > > > The updatePersistentNode() part isn't vital, it is mostly in place to
> > handle
> > > > > some untidy Persistent<> ownership details for one or two objects used
> > > across
> > > > > threads. I can explain further tomorrow.
> > > > > 
> > > > > But, assuming you only clear a Persistent<> at the end of its
lifetime,
> it
> > > > > doesn't have considerable overhead.
> > > > 
> > > > That comment is a bit confusingly worded -- updatePersistentNode() uses
> upon
> > > > updating m_raw (not setting it initially) aren't vital.
> > > 
> > > Yes, I agree that it would be rare. What I'm a bit concerned about is the
> > > overhead of the if branches inserted into updatePersistentNode(). (My
> > intuition
> > > is that the overhead wouldn't matter but want to confirm that it actually
> > > doesn't matter.)
> > 
> > We'll see. But it is worth keeping in mind the amount of alloc/free work
> > currently done for the PersistentNodes numbers quoted on
> > 
> > 
> >
>
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/6r4ei5OESdc/qjT...
> 
> I considered a bit more and agree with you that this CL will improve
performance
> in theory.
> 
> - The if branches will regress performance of operations on non-empty
persistent
> handles by X%.
> - The if branches will improve performance of operations on empty persistent
> handles by Y%.
> - Y would be larger than X.
> - The number of empty persistent handles is larger than the number of
non-empty
> persistent handles.
> - In conclusion, this CL will improve performance overall.
> 
> Am I making sense?

It does make sense to me. The theory will not hold if we have large amounts of
writes/updates to Persistent<>s in some perf sensitive code - i.e., this would
be akin to write barrier overheads.

I will add some instrumentation to gauge how Persistent<>s are used & get a
handle on the ratio between initializing and updating Persistent<>s.

[haraken, 2015-09-13 14:53:33]

On 2015/09/13 14:46:55, sof wrote:
> On 2015/09/13 13:35:15, haraken wrote:
> > On 2015/09/13 13:27:03, sof wrote:
> > > On 2015/09/13 13:20:35, haraken wrote:
> > > > On 2015/09/13 12:50:25, sof wrote:
> > > > > On 2015/09/13 12:47:46, sof wrote:
> > > > > > On 2015/09/13 12:36:56, haraken wrote:
> > > > > > > I like this change but am a bit concerned about a performance
> impact,
> > > > since
> > > > > > the
> > > > > > > CL is inserting updatePersistentNode() into performance-sensitive
> > > methods.
> > > > > > > 
> > > > > > 
> > > > > > The updatePersistentNode() part isn't vital, it is mostly in place
to
> > > handle
> > > > > > some untidy Persistent<> ownership details for one or two objects
used
> > > > across
> > > > > > threads. I can explain further tomorrow.
> > > > > > 
> > > > > > But, assuming you only clear a Persistent<> at the end of its
> lifetime,
> > it
> > > > > > doesn't have considerable overhead.
> > > > > 
> > > > > That comment is a bit confusingly worded -- updatePersistentNode()
uses
> > upon
> > > > > updating m_raw (not setting it initially) aren't vital.
> > > > 
> > > > Yes, I agree that it would be rare. What I'm a bit concerned about is
the
> > > > overhead of the if branches inserted into updatePersistentNode(). (My
> > > intuition
> > > > is that the overhead wouldn't matter but want to confirm that it
actually
> > > > doesn't matter.)
> > > 
> > > We'll see. But it is worth keeping in mind the amount of alloc/free work
> > > currently done for the PersistentNodes numbers quoted on
> > > 
> > > 
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/6r4ei5OESdc/qjT...
> > 
> > I considered a bit more and agree with you that this CL will improve
> performance
> > in theory.
> > 
> > - The if branches will regress performance of operations on non-empty
> persistent
> > handles by X%.
> > - The if branches will improve performance of operations on empty persistent
> > handles by Y%.
> > - Y would be larger than X.
> > - The number of empty persistent handles is larger than the number of
> non-empty
> > persistent handles.
> > - In conclusion, this CL will improve performance overall.
> > 
> > Am I making sense?
> 
> It does make sense to me. The theory will not hold if we have large amounts of
> writes/updates to Persistent<>s in some perf sensitive code - i.e., this would
> be akin to write barrier overheads.
> 
> I will add some instrumentation to gauge how Persistent<>s are used & get a
> handle on the ratio between initializing and updating Persistent<>s.

Yeah, I'm now convinced that this CL will be a win. Getting some numbers is
useful :)

[commit-bot: I haz the power, 2015-09-13 15:17:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-13 15:17:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2015-09-13 20:46:07]

On 2015/09/13 14:53:33, haraken wrote:
> On 2015/09/13 14:46:55, sof wrote:
> > On 2015/09/13 13:35:15, haraken wrote:
> > > On 2015/09/13 13:27:03, sof wrote:
> > > > On 2015/09/13 13:20:35, haraken wrote:
> > > > > On 2015/09/13 12:50:25, sof wrote:
> > > > > > On 2015/09/13 12:47:46, sof wrote:
> > > > > > > On 2015/09/13 12:36:56, haraken wrote:
> > > > > > > > I like this change but am a bit concerned about a performance
> > impact,
> > > > > since
> > > > > > > the
> > > > > > > > CL is inserting updatePersistentNode() into
performance-sensitive
> > > > methods.
> > > > > > > > 
> > > > > > > 
> > > > > > > The updatePersistentNode() part isn't vital, it is mostly in place
> to
> > > > handle
> > > > > > > some untidy Persistent<> ownership details for one or two objects
> used
> > > > > across
> > > > > > > threads. I can explain further tomorrow.
> > > > > > > 
> > > > > > > But, assuming you only clear a Persistent<> at the end of its
> > lifetime,
> > > it
> > > > > > > doesn't have considerable overhead.
> > > > > > 
> > > > > > That comment is a bit confusingly worded -- updatePersistentNode()
> uses
> > > upon
> > > > > > updating m_raw (not setting it initially) aren't vital.
> > > > > 
> > > > > Yes, I agree that it would be rare. What I'm a bit concerned about is
> the
> > > > > overhead of the if branches inserted into updatePersistentNode(). (My
> > > > intuition
> > > > > is that the overhead wouldn't matter but want to confirm that it
> actually
> > > > > doesn't matter.)
> > > > 
> > > > We'll see. But it is worth keeping in mind the amount of alloc/free work
> > > > currently done for the PersistentNodes numbers quoted on
> > > > 
> > > > 
> > > >
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/6r4ei5OESdc/qjT...
> > > 
> > > I considered a bit more and agree with you that this CL will improve
> > performance
> > > in theory.
> > > 
> > > - The if branches will regress performance of operations on non-empty
> > persistent
> > > handles by X%.
> > > - The if branches will improve performance of operations on empty
persistent
> > > handles by Y%.
> > > - Y would be larger than X.
> > > - The number of empty persistent handles is larger than the number of
> > non-empty
> > > persistent handles.
> > > - In conclusion, this CL will improve performance overall.
> > > 
> > > Am I making sense?
> > 
> > It does make sense to me. The theory will not hold if we have large amounts
of
> > writes/updates to Persistent<>s in some perf sensitive code - i.e., this
would
> > be akin to write barrier overheads.
> > 
> > I will add some instrumentation to gauge how Persistent<>s are used & get a
> > handle on the ratio between initializing and updating Persistent<>s.
> 
> Yeah, I'm now convinced that this CL will be a win. Getting some numbers is
> useful :)

dom-modify is a test that allocates PersistentNodes a fair bit across its
subtests,

 - allocates some 3M PersistentNodes while running.
 - ~4800 calls to updatePersistentNode() over those.
 - ~1k of those updates are identical pointer value updates by
FontFallbackList::invalidate(). (We should make that assignment conditional.)
 - if we ignore the previous, _one_ PersistentNode is updated 3 times, 550 are
updated twice, the rest only once.

ToT allocates 6.3M PersistentNodes.

Other tests (and sites) show up a similar pattern -- very few PersistentNodes
are updated (and multiple times), meaning that most times you allocate the
PersistentNodes when initializing the PersistentBase with a non-null value.
Multiple updates are even rarer, hence the additional two pointer null checks
required to handle those are nothing to be concerned with in practice.

[haraken, 2015-09-13 23:23:24]

On 2015/09/13 20:46:07, sof wrote:
> On 2015/09/13 14:53:33, haraken wrote:
> > On 2015/09/13 14:46:55, sof wrote:
> > > On 2015/09/13 13:35:15, haraken wrote:
> > > > On 2015/09/13 13:27:03, sof wrote:
> > > > > On 2015/09/13 13:20:35, haraken wrote:
> > > > > > On 2015/09/13 12:50:25, sof wrote:
> > > > > > > On 2015/09/13 12:47:46, sof wrote:
> > > > > > > > On 2015/09/13 12:36:56, haraken wrote:
> > > > > > > > > I like this change but am a bit concerned about a performance
> > > impact,
> > > > > > since
> > > > > > > > the
> > > > > > > > > CL is inserting updatePersistentNode() into
> performance-sensitive
> > > > > methods.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > The updatePersistentNode() part isn't vital, it is mostly in
place
> > to
> > > > > handle
> > > > > > > > some untidy Persistent<> ownership details for one or two
objects
> > used
> > > > > > across
> > > > > > > > threads. I can explain further tomorrow.
> > > > > > > > 
> > > > > > > > But, assuming you only clear a Persistent<> at the end of its
> > > lifetime,
> > > > it
> > > > > > > > doesn't have considerable overhead.
> > > > > > > 
> > > > > > > That comment is a bit confusingly worded -- updatePersistentNode()
> > uses
> > > > upon
> > > > > > > updating m_raw (not setting it initially) aren't vital.
> > > > > > 
> > > > > > Yes, I agree that it would be rare. What I'm a bit concerned about
is
> > the
> > > > > > overhead of the if branches inserted into updatePersistentNode().
(My
> > > > > intuition
> > > > > > is that the overhead wouldn't matter but want to confirm that it
> > actually
> > > > > > doesn't matter.)
> > > > > 
> > > > > We'll see. But it is worth keeping in mind the amount of alloc/free
work
> > > > > currently done for the PersistentNodes numbers quoted on
> > > > > 
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/6r4ei5OESdc/qjT...
> > > > 
> > > > I considered a bit more and agree with you that this CL will improve
> > > performance
> > > > in theory.
> > > > 
> > > > - The if branches will regress performance of operations on non-empty
> > > persistent
> > > > handles by X%.
> > > > - The if branches will improve performance of operations on empty
> persistent
> > > > handles by Y%.
> > > > - Y would be larger than X.
> > > > - The number of empty persistent handles is larger than the number of
> > > non-empty
> > > > persistent handles.
> > > > - In conclusion, this CL will improve performance overall.
> > > > 
> > > > Am I making sense?
> > > 
> > > It does make sense to me. The theory will not hold if we have large
amounts
> of
> > > writes/updates to Persistent<>s in some perf sensitive code - i.e., this
> would
> > > be akin to write barrier overheads.
> > > 
> > > I will add some instrumentation to gauge how Persistent<>s are used & get
a
> > > handle on the ratio between initializing and updating Persistent<>s.
> > 
> > Yeah, I'm now convinced that this CL will be a win. Getting some numbers is
> > useful :)
> 
> dom-modify is a test that allocates PersistentNodes a fair bit across its
> subtests,
> 
>  - allocates some 3M PersistentNodes while running.
>  - ~4800 calls to updatePersistentNode() over those.
>  - ~1k of those updates are identical pointer value updates by
> FontFallbackList::invalidate(). (We should make that assignment conditional.)
>  - if we ignore the previous, _one_ PersistentNode is updated 3 times, 550 are
> updated twice, the rest only once.
> 
> ToT allocates 6.3M PersistentNodes.
> 
> Other tests (and sites) show up a similar pattern -- very few PersistentNodes
> are updated (and multiple times), meaning that most times you allocate the
> PersistentNodes when initializing the PersistentBase with a non-null value.
> Multiple updates are even rarer, hence the additional two pointer null checks
> required to handle those are nothing to be concerned with in practice.

Thanks for the numbers! LGTM once the comments on PS2 are addressed.

[haraken, 2015-09-14 01:02:55]

On 2015/09/13 23:23:24, haraken wrote:
> On 2015/09/13 20:46:07, sof wrote:
> > On 2015/09/13 14:53:33, haraken wrote:
> > > On 2015/09/13 14:46:55, sof wrote:
> > > > On 2015/09/13 13:35:15, haraken wrote:
> > > > > On 2015/09/13 13:27:03, sof wrote:
> > > > > > On 2015/09/13 13:20:35, haraken wrote:
> > > > > > > On 2015/09/13 12:50:25, sof wrote:
> > > > > > > > On 2015/09/13 12:47:46, sof wrote:
> > > > > > > > > On 2015/09/13 12:36:56, haraken wrote:
> > > > > > > > > > I like this change but am a bit concerned about a
performance
> > > > impact,
> > > > > > > since
> > > > > > > > > the
> > > > > > > > > > CL is inserting updatePersistentNode() into
> > performance-sensitive
> > > > > > methods.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > The updatePersistentNode() part isn't vital, it is mostly in
> place
> > > to
> > > > > > handle
> > > > > > > > > some untidy Persistent<> ownership details for one or two
> objects
> > > used
> > > > > > > across
> > > > > > > > > threads. I can explain further tomorrow.
> > > > > > > > > 
> > > > > > > > > But, assuming you only clear a Persistent<> at the end of its
> > > > lifetime,
> > > > > it
> > > > > > > > > doesn't have considerable overhead.
> > > > > > > > 
> > > > > > > > That comment is a bit confusingly worded --
updatePersistentNode()
> > > uses
> > > > > upon
> > > > > > > > updating m_raw (not setting it initially) aren't vital.
> > > > > > > 
> > > > > > > Yes, I agree that it would be rare. What I'm a bit concerned about
> is
> > > the
> > > > > > > overhead of the if branches inserted into updatePersistentNode().
> (My
> > > > > > intuition
> > > > > > > is that the overhead wouldn't matter but want to confirm that it
> > > actually
> > > > > > > doesn't matter.)
> > > > > > 
> > > > > > We'll see. But it is worth keeping in mind the amount of alloc/free
> work
> > > > > > currently done for the PersistentNodes numbers quoted on
> > > > > > 
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/oilpan-reviews/6r4ei5OESdc/qjT...
> > > > > 
> > > > > I considered a bit more and agree with you that this CL will improve
> > > > performance
> > > > > in theory.
> > > > > 
> > > > > - The if branches will regress performance of operations on non-empty
> > > > persistent
> > > > > handles by X%.
> > > > > - The if branches will improve performance of operations on empty
> > persistent
> > > > > handles by Y%.
> > > > > - Y would be larger than X.
> > > > > - The number of empty persistent handles is larger than the number of
> > > > non-empty
> > > > > persistent handles.
> > > > > - In conclusion, this CL will improve performance overall.
> > > > > 
> > > > > Am I making sense?
> > > > 
> > > > It does make sense to me. The theory will not hold if we have large
> amounts
> > of
> > > > writes/updates to Persistent<>s in some perf sensitive code - i.e., this
> > would
> > > > be akin to write barrier overheads.
> > > > 
> > > > I will add some instrumentation to gauge how Persistent<>s are used &
get
> a
> > > > handle on the ratio between initializing and updating Persistent<>s.
> > > 
> > > Yeah, I'm now convinced that this CL will be a win. Getting some numbers
is
> > > useful :)
> > 
> > dom-modify is a test that allocates PersistentNodes a fair bit across its
> > subtests,
> > 
> >  - allocates some 3M PersistentNodes while running.
> >  - ~4800 calls to updatePersistentNode() over those.
> >  - ~1k of those updates are identical pointer value updates by
> > FontFallbackList::invalidate(). (We should make that assignment
conditional.)
> >  - if we ignore the previous, _one_ PersistentNode is updated 3 times, 550
are
> > updated twice, the rest only once.
> > 
> > ToT allocates 6.3M PersistentNodes.
> > 
> > Other tests (and sites) show up a similar pattern -- very few
PersistentNodes
> > are updated (and multiple times), meaning that most times you allocate the
> > PersistentNodes when initializing the PersistentBase with a non-null value.
> > Multiple updates are even rarer, hence the additional two pointer null
checks
> > required to handle those are nothing to be concerned with in practice.
> 
> Thanks for the numbers! LGTM once the comments on PS2 are addressed.

(Also please add more explanations to the CL description.)

[sof, 2015-09-14 05:35:39]

On 2015/09/13 12:50:25, sof wrote:
> On 2015/09/13 12:47:46, sof wrote:
> > On 2015/09/13 12:36:56, haraken wrote:
> > > I like this change but am a bit concerned about a performance impact,
since
> > the
> > > CL is inserting updatePersistentNode() into performance-sensitive methods.
> > > 
> > 
> > The updatePersistentNode() part isn't vital, it is mostly in place to handle
> > some untidy Persistent<> ownership details for one or two objects used
across
> > threads. I can explain further tomorrow.
> > 
> > But, assuming you only clear a Persistent<> at the end of its lifetime, it
> > doesn't have considerable overhead.
> 
> That comment is a bit confusingly worded -- updatePersistentNode() uses upon
> updating m_raw (not setting it initially) aren't vital.

Returning to this one -- the reason why we have uninitialize() in
updatePersistentNode() is to handle thread shutdowns cleanly. It shows up for
WorkerThread, but the details are simpler for DataConsumerHandleTestUtil (but
wieldy enough):

 - a DataConsumerHandleTestUtil::Thread is created on the main thread &
initiated.
 - as part of initializing it creates its m_executionContext
NullExecutionContext, on the thread.
 - this is a Persistent<>, so if we do lazy creation of PersistentNodes, it will
be registered with the thread's persistent region.
 - DataConsumerHandleTestUtil::Thread::shutdown() clears its m_executionContext
before shutting down the thread.
 - if clearing m_executionContext doesn't unregister the PersistentNode, the
thread shutdown will fail to clean out the thread persistents & the shutdown
assert in ThreadState::cleanup() triggers.
 - which is why updatePersistentNode() calls uninitialize(); otherwise the
Persistent<> will live until DataConsumerHandleTestUtil::Thread is destructed,
which happens after thread shutdown.

With the ToT scheme, the Persistent<> will be allocated/registered on the main
thread, so this issue doesn't arise during thread shutdown.

Does all those details explain & make some kind of sense?

[I'm in two minds if this PersistentNode "unregistration" is something we should
provide as a separate Persistent<> operation, or bake it into
updatePersistentNode().]

[sof, 2015-09-14 06:34:04]

btw, SelfKeepAlive<> can be simplified following this one -- it no longer needs
to maintain an OwnPtr<Persistent<T>>

https://codereview.chromium.org/1338573003/diff/20001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1338573003/diff/20001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:216: void updatePersistentNode()
On 2015/09/13 12:36:56, haraken wrote:
> 
> Maybe I'd prefer renaming this to assign() and do 'm_raw = ...' in assign().
> 
> void assign(T* value) {
>   m_raw = value;
>   if (m_raw)
>     ...;
> }
> 
> Also you can move checkPointer() into assign().

Switched to that scheme; I think it is ok to leave it to the compiler whether or
not to inline.

https://codereview.chromium.org/1338573003/diff/20001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:224: uninitialize();
On 2015/09/13 12:36:56, haraken wrote:
> 
> Will this remove the restriction that a (non-cross-thread) persistent handle
> must be destructed before the thread shuts down? After this CL, the thread can
> shut down if the persistent handle is cleared (doesn't need to be destructed).

It removes that restriction, as detailed on
https://codereview.chromium.org/1338573003/#msg17

https://codereview.chromium.org/1338573003/diff/20001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:230: if (m_persistentNode || !m_raw)
On 2015/09/13 12:36:56, haraken wrote:
> 
> Is it possible that m_persistentNode is non-null here?

It no longer is, thanks - replaced with an assert.

[sof, 2015-09-14 06:34:37]

On 2015/09/14 01:02:55, haraken wrote:
>
...
> 
> (Also please add more explanations to the CL description.)

A reasonable request :) Done.

[haraken, 2015-09-14 06:43:07]

LGTM

[sof, 2015-09-14 09:51:59]

https://codereview.chromium.org/1338573003/diff/40001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1338573003/diff/40001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:212: ASSERT(!m_persistentNode);
Of course we need a !m_raw test here still! :)

[sof, 2015-09-14 11:52:09]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-09-14 11:52:09]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1338573003/#ps60001
(title: "add back missing m_raw nullcheck that ps#3 incorrectly dropped")

[commit-bot: I haz the power, 2015-09-14 11:52:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1338573003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1338573003/60001

[commit-bot: I haz the power, 2015-09-14 11:56:59]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202197

[commit-bot: I haz the power, 2015-09-23 12:31:43]

Patchset 4 (id:??) landed as
https://crrev.com/b119d7e4f458aeaf3d53c5e50facac4abf4a2742