Cache element indices for :nth-child and :nth-last-child selectors.

Cache element indices for :nth-child and :nth-last-child selectors.

In order to avoid n^2 for :nth-selectors, we introduce a cache where we
store the index of every kth child of a parent node P the first time the
nth-count is queried for one of its children. The number k is given by
the "spread" constant.

After the cache has been populated for the children of P, the nth-index
for an element will be found by walking the siblings from the element
queried for and leftwards until a cached element/index pair is found.
So populating the cache for P is O(n). Subsequent lookups are best case
O(1), worst case O(k).

The cache is created on the stack when we do operations where we know we
can benefit from having it. Currently, those are querySelector,
querySelectorAll, and updateStyle. We are throwing away the cache after
each operation to avoid holding on to potentially large caches in memory.

R=esprehn@chromium.org
BUG=461878
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193268

[rune, 2015-03-26 11:16:16]

rune@opera.com changed reviewers:
+ esprehn@chromium.org

[rune, 2015-03-26 11:21:17]

rune@opera.com changed reviewers:
+ sigbjornf@opera.com

[rune, 2015-03-26 11:21:17]

sof: could you take a look at the oilpanification of NthIndexCache and
NthIndexData to see if it looks sensible?

[rune, 2015-03-27 14:10:02]

In patch set 7, we now clear the cache after each
querySelector/querySelectorAll/updateRenderTree.

[sof, 2015-03-28 08:34:00]

Sorry, this CL escaped my attention yesterday -- some Oilpan feedback.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:666: m_nthIndexCache = adoptPtr(new
NthIndexCache(*this));
adoptPtrWillBeNoop() -- would be tidier to provide
NthIndexCache::create(Document&)

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:1067: public:
If you add STACK_ALLOCATED(); here and turn the Document& reference into a
RawPtrWillBeMember<Document>, the scope object will be handled as expected
during conservative stack scanning.

(It's a somewhat specialized scope object to expose via Document, so perhaps
consider lifting it out into a separate file?)

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:1339: OwnPtrWillBeMember<NthIndexCache>
m_nthIndexCache;
Add a trace() call for this to Document's trace() implementation.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCache.h (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:51: typedef
WillBeHeapHashMap<RefPtrWillBeMember<Node>, OwnPtrWillBeMember<NthIndexData>>
ParentMap;
nit: "using" is preferred over "typedef" now.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:54: Document& m_document;
Make this a RawPtrWillBeMember<Document> and have it be traced.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCacheTest.cpp (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCacheTest.cpp:33: m_document =
&m_dummyPageHolder->document();
Worth caching this Document* reference?

[rune, 2015-03-30 09:00:03]

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:666: m_nthIndexCache = adoptPtr(new
NthIndexCache(*this));
On 2015/03/28 08:33:59, sof wrote:
> adoptPtrWillBeNoop() -- would be tidier to provide
> NthIndexCache::create(Document&)

Done.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:1067: public:
On 2015/03/28 08:33:59, sof wrote:
> If you add STACK_ALLOCATED(); here and turn the Document& reference into a
> RawPtrWillBeMember<Document>, the scope object will be handled as expected
> during conservative stack scanning.

Done.

> (It's a somewhat specialized scope object to expose via Document, so perhaps
> consider lifting it out into a separate file?)

It only makes sense in the context of Document as it's about temporarily
changing the state of Document, and I wanted to make sure setUseNthIndexCache is
not used by other classes, at least not outside of Document, thus making it a
member class. The alternative would be to make it a friend of Document.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:1339: OwnPtrWillBeMember<NthIndexCache>
m_nthIndexCache;
On 2015/03/28 08:33:59, sof wrote:
> Add a trace() call for this to Document's trace() implementation.

Done.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCache.h (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:51: typedef
WillBeHeapHashMap<RefPtrWillBeMember<Node>, OwnPtrWillBeMember<NthIndexData>>
ParentMap;
On 2015/03/28 08:33:59, sof wrote:
> nit: "using" is preferred over "typedef" now. 

Done.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:54: Document& m_document;
On 2015/03/28 08:33:59, sof wrote:
> Make this a RawPtrWillBeMember<Document> and have it be traced.

Done.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCacheTest.cpp (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCacheTest.cpp:33: m_document =
&m_dummyPageHolder->document();
On 2015/03/28 08:34:00, sof wrote:
> Worth caching this Document* reference?

Done.

[sof, 2015-03-30 13:27:53]

oilpan lgtm

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.h:1067: public:
On 2015/03/30 09:00:03, rune wrote:
> On 2015/03/28 08:33:59, sof wrote:
>
...
> 
> > (It's a somewhat specialized scope object to expose via Document, so perhaps
> > consider lifting it out into a separate file?)
> 
> It only makes sense in the context of Document as it's about temporarily
> changing the state of Document, and I wanted to make sure setUseNthIndexCache
is
> not used by other classes, at least not outside of Document, thus making it a
> member class. The alternative would be to make it a friend of Document.

I won't insist, but having the scope object be a friend would look tidier. It
wouldn't be out of place to expose UseNthIndexCacheScope via NthIndexCache.h

https://codereview.chromium.org/1023393002/diff/140001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1023393002/diff/140001/Source/core/dom/Docume...
Source/core/dom/Document.h:1423: bool useNthIndexCache() const { return
m_useNthIndexCache; }
Let's move up the accessors, away from the field declarations.

https://codereview.chromium.org/1023393002/diff/160001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1023393002/diff/160001/Source/core/dom/Docume...
Source/core/dom/Document.h:1073: ASSERT(!document.useNthIndexCache());
Just to verify: there's no possibility that you may ever enter nested scopes?

[rune, 2015-03-30 20:14:40]

So, I've moved the NthIndexCacheScope to NthIndexCache.h and made it a friend of
Document.

https://codereview.chromium.org/1023393002/diff/140001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1023393002/diff/140001/Source/core/dom/Docume...
Source/core/dom/Document.h:1423: bool useNthIndexCache() const { return
m_useNthIndexCache; }
On 2015/03/30 13:27:53, sof wrote:
> Let's move up the accessors, away from the field declarations.

Done.

https://codereview.chromium.org/1023393002/diff/160001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1023393002/diff/160001/Source/core/dom/Docume...
Source/core/dom/Document.h:1073: ASSERT(!document.useNthIndexCache());
On 2015/03/30 13:27:53, sof wrote:
> Just to verify: there's no possibility that you may ever enter nested scopes?

Shouldn't with the current uses of this scope. querySelector(All) should not
nest, and the DocumentLifecycle should ensure we don't nest updateStyle. The
assert here is to introduce future nesting problems. I'd rather avoid nested
scopes than mending them by checking/storing the current useNthIndexCache().

[sof, 2015-03-30 20:55:17]

Thanks for moving the scope object.

Sorry about the late double-checking, but noticed a transition type missing. Add
a couple of nits while at it.

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1023393002/diff/120001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:666: m_nthIndexCache = adoptPtr(new
NthIndexCache(*this));
On 2015/03/30 09:00:03, rune wrote:
> On 2015/03/28 08:33:59, sof wrote:
> > adoptPtrWillBeNoop() -- would be tidier to provide
> > NthIndexCache::create(Document&)
> 
> Done.

Oh, not quite, you're still using adoptPtr() -- ought to be

  static PassOwnPtrWillBeRawPtr<NthIndexCache> create(Document& document)
  {
      return adoptPtrWillBeNoop(new NthIndexCache(document));
  }

https://codereview.chromium.org/1023393002/diff/180001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCache.h (right):

https://codereview.chromium.org/1023393002/diff/180001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:19: class NthIndexCache :
NoBaseWillBeGarbageCollected<NthIndexCache> {
nit: add final.

https://codereview.chromium.org/1023393002/diff/180001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:26: 
nit: it wouldn't hurt to define a dtor for non-Oilpan -- add

  DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(NthIndexCache);

along with

  DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(NthIndexCache);

in the implementation file.

https://codereview.chromium.org/1023393002/diff/180001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:35: NthIndexCache(Document& document)
nit: add explicit

[rune, 2015-03-30 22:01:45]

https://codereview.chromium.org/1023393002/diff/180001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCache.h (right):

https://codereview.chromium.org/1023393002/diff/180001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:19: class NthIndexCache :
NoBaseWillBeGarbageCollected<NthIndexCache> {
On 2015/03/30 20:55:17, sof wrote:
> nit: add final.

Done. Same for NthIndexData.

https://codereview.chromium.org/1023393002/diff/180001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:26: 
On 2015/03/30 20:55:17, sof wrote:
> nit: it wouldn't hurt to define a dtor for non-Oilpan -- add
> 
>   DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(NthIndexCache);
> 
> along with
> 
>   DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(NthIndexCache);
> 
> in the implementation file.

Done. Did the same for NthIndexData, but had to hand-write the impl.

https://codereview.chromium.org/1023393002/diff/180001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:35: NthIndexCache(Document& document)
On 2015/03/30 20:55:17, sof wrote:
> nit: add explicit

Done.

[esprehn, 2015-03-30 22:55:31]

Instead of all this adding and clearing I'd just use the stack to manage this:


static NthIndexCache* s_cache = 0;

class NthIndexCache {
  NthIndexCache(Document& document)
   : m_document(document),
     m_domTreeVersion(document.domTreeVersion()) {
    ASSERT(!s_cache);
    s_cache = this;;
  }
  ~NthIndexCache {
    // Forbid mutations while a cache exists.
    ASSERT(m_domTreeVersion == document.domTreeVersion());
    s_cache = nullptr;
  }

  instance() {
    return s_cache;
  }
}

Then you put one on the stack at the start of Document::updateStyle and the
start of the querySelector code.

https://codereview.chromium.org/1023393002/diff/200001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1023393002/diff/200001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:661: NthIndexCache* Document::nthIndexCache()
Instead of putting this on the Document, lets keep a global static of it.

https://codereview.chromium.org/1023393002/diff/200001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:670: void Document::setUseNthIndexCache(bool use)
remove this.

https://codereview.chromium.org/1023393002/diff/200001/Source/core/dom/Docume...
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/1023393002/diff/200001/Source/core/dom/Docume...
Source/core/dom/Document.h:1179: void setUseNthIndexCache(bool);
This is the same as null checking the OwnPtr. You don't need the bool.

[rune, 2015-03-30 23:39:36]

On 2015/03/30 22:55:31, esprehn wrote:
> Instead of all this adding and clearing I'd just use the stack to manage this:
> 
> static NthIndexCache* s_cache = 0;
> 
> class NthIndexCache {
>   NthIndexCache(Document& document)
>    : m_document(document),
>      m_domTreeVersion(document.domTreeVersion()) {
>     ASSERT(!s_cache);
>     s_cache = this;;
>   }
>   ~NthIndexCache {
>     // Forbid mutations while a cache exists.
>     ASSERT(m_domTreeVersion == document.domTreeVersion());
>     s_cache = nullptr;
>   }
> 
>   instance() {
>     return s_cache;
>   }
> }
> 
> Then you put one on the stack at the start of Document::updateStyle and the
> start of the querySelector code.

I'm confused about oilpan. Making NthIndexCache stack-allocated means I can't
keep pointers to it, I've been told. I guess it really means that I cannot
Member<> them. I find it really hard to figure out what's ok and not in the
oilpan world. I guess that storing a global pointer to a stack allocated object
that is reset when the stack allocated goes out of scope won't go wrong wrt
garbage collection?

[esprehn, 2015-03-31 00:14:52]

On 2015/03/30 at 23:39:36, rune wrote:
> On 2015/03/30 22:55:31, esprehn wrote:
> > ..
> > Then you put one on the stack at the start of Document::updateStyle and the
> > start of the querySelector code.
> 
> I'm confused about oilpan. Making NthIndexCache stack-allocated means I can't
keep pointers to it, I've been told. I guess it really means that I cannot
Member<> them. I find it really hard to figure out what's ok and not in the
oilpan world. I guess that storing a global pointer to a stack allocated object
that is reset when the stack allocated goes out of scope won't go wrong wrt
garbage collection?

It probably needs to be a Persistent<>?

[sof, 2015-03-31 09:48:29]

On 2015/03/30 23:39:36, rune wrote:
> On 2015/03/30 22:55:31, esprehn wrote:
> > Instead of all this adding and clearing I'd just use the stack to manage
this:
> > 
> > static NthIndexCache* s_cache = 0;
> > 
> > class NthIndexCache {
> >   NthIndexCache(Document& document)
> >    : m_document(document),
> >      m_domTreeVersion(document.domTreeVersion()) {
> >     ASSERT(!s_cache);
> >     s_cache = this;;
> >   }
> >   ~NthIndexCache {
> >     // Forbid mutations while a cache exists.
> >     ASSERT(m_domTreeVersion == document.domTreeVersion());
> >     s_cache = nullptr;
> >   }
> > 
> >   instance() {
> >     return s_cache;
> >   }
> > }
> > 
> > Then you put one on the stack at the start of Document::updateStyle and the
> > start of the querySelector code.
> 
> I'm confused about oilpan. Making NthIndexCache stack-allocated means I can't
> keep pointers to it, I've been told. I guess it really means that I cannot
> Member<> them. I find it really hard to figure out what's ok and not in the
> oilpan world. I guess that storing a global pointer to a stack allocated
object
> that is reset when the stack allocated goes out of scope won't go wrong wrt
> garbage collection?

You cannot keep traceable references to it, i.e., consider it like an Oilpan
heap allocated object with its prefixed header needed by the garbage collector.

Keeping a raw, shared pointer to a stack allocated object that you clear upon
deallocation, is not a problem. The global pointer provides the sharing, the
stack&Oilpan takes care of keeping the NthIndexCache-embedded heap references
alive across a (conservative) GC, should that happen.

[rune, 2015-03-31 11:04:56]

Now back to a stack-based cache ...

[sof, 2015-03-31 16:59:04]

On 2015/03/31 11:04:56, rune wrote:
> Now back to a stack-based cache ...

..and let's assume no two Documents ever live together.

(ps#12 needs a rebase.)

[sof, 2015-03-31 16:59:21]

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCache.h (right):

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:26: static NthIndexCache* getInstance() { return
s_instance; }
nit: instance() (or current())

[esprehn, 2015-03-31 17:35:15]

lgtm w/ nits. Thanks for fixing this!

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCache.cpp (right):

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.cpp:44: const unsigned spread = 3;
You might want a comment that explains what spread does.

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCache.h (right):

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:23: NthIndexCache(Document&);
explicit

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:26: static NthIndexCache* getInstance() { return
s_instance; }
instance()

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:36: NthIndexData() : m_count(0) { }
I think you can actually do m_count = 0; below now and omit this constructor.

[rune, 2015-03-31 20:57:37]

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCache.cpp (right):

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.cpp:44: const unsigned spread = 3;
On 2015/03/31 17:35:15, esprehn wrote:
> You might want a comment that explains what spread does.

Done.

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
File Source/core/dom/NthIndexCache.h (right):

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:23: NthIndexCache(Document&);
On 2015/03/31 17:35:15, esprehn wrote:
> explicit

Done.

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:26: static NthIndexCache* getInstance() { return
s_instance; }
On 2015/03/31 17:35:15, esprehn wrote:
> instance()

Done.

https://codereview.chromium.org/1023393002/diff/220001/Source/core/dom/NthInd...
Source/core/dom/NthIndexCache.h:36: NthIndexData() : m_count(0) { }
On 2015/03/31 17:35:15, esprehn wrote:
> I think you can actually do m_count = 0; below now and omit this constructor.

Done. I had to keep the constructor since there's a copy one declared by
WTF_MAKE_NONCOPYABLE, so I don't know if it was worth it.

[rune, 2015-03-31 20:57:48]

The CQ bit was checked by rune@opera.com

[rune, 2015-03-31 20:57:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1023393002/#ps260001
(title: "Rebased")

[commit-bot: I haz the power, 2015-03-31 20:59:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023393002/260001

[rune, 2015-03-31 22:24:34]

On 2015/03/31 16:59:04, sof wrote:
> On 2015/03/31 11:04:56, rune wrote:
> > Now back to a stack-based cache ...
> 
> ..and let's assume no two Documents ever live together.

This seems to be a problem, yes. SVGs as data URIs for background-image and
shape-outside seems to be parsed and attached synchronously from style update
for the document they're used by (I've not debugged, based on bot-output).

Back to storing on Document, or untangling the synchronous loading?

[rune, 2015-03-31 22:24:47]

The CQ bit was unchecked by rune@opera.com

[esprehn, 2015-03-31 22:47:05]

On 2015/03/31 at 22:24:34, rune wrote:
> On 2015/03/31 16:59:04, sof wrote:
> > On 2015/03/31 11:04:56, rune wrote:
> > > Now back to a stack-based cache ...
> > 
> > ..and let's assume no two Documents ever live together.
> 
> This seems to be a problem, yes. SVGs as data URIs for background-image and
shape-outside seems to be parsed and attached synchronously from style update
for the document they're used by (I've not debugged, based on bot-output).
> 
> Back to storing on Document, or untangling the synchronous loading?

I'd go back to storing on the document, but just do it in the constructor where
you put it on the stack.

NthCache cache(document)

NthCache's constructor does document.setNthCache(this), and document does
ASSERT(!m_nthCache) so you can't overwrite an existing one. I'd also leave the
domTreeVersion assert in the NthCache destructor to make sure a cache can never
be invalidated in the middle.

[esprehn, 2015-03-31 22:47:14]

Also sorry for all the iteration on this :(

[rune, 2015-04-04 14:28:35]

On 2015/03/31 22:47:05, esprehn wrote:
> On 2015/03/31 at 22:24:34, rune wrote:
> > On 2015/03/31 16:59:04, sof wrote:
> > > On 2015/03/31 11:04:56, rune wrote:
> > > > Now back to a stack-based cache ...
> > > 
> > > ..and let's assume no two Documents ever live together.
> > 
> > This seems to be a problem, yes. SVGs as data URIs for background-image and
> shape-outside seems to be parsed and attached synchronously from style update
> for the document they're used by (I've not debugged, based on bot-output).
> > 
> > Back to storing on Document, or untangling the synchronous loading?
> 
> I'd go back to storing on the document, but just do it in the constructor
where
> you put it on the stack.
> 
> NthCache cache(document)

Which doesn't work because Document is a heap-allocated GCed object which cannot
contain a raw ptr to a stack allocated GCed object, the clang oilpan plugin
tells me. :-(

> NthCache's constructor does document.setNthCache(this), and document does
> ASSERT(!m_nthCache) so you can't overwrite an existing one. I'd also leave the
> domTreeVersion assert in the NthCache destructor to make sure a cache can
never
> be invalidated in the middle.

Yup.

[sof, 2015-04-04 18:29:06]

On 2015/04/04 14:28:35, OOO (back 2015-04-07) wrote:
> On 2015/03/31 22:47:05, esprehn wrote:
> > On 2015/03/31 at 22:24:34, rune wrote:
> > > On 2015/03/31 16:59:04, sof wrote:
> > > > On 2015/03/31 11:04:56, rune wrote:
> > > > > Now back to a stack-based cache ...
> > > > 
> > > > ..and let's assume no two Documents ever live together.
> > > 
> > > This seems to be a problem, yes. SVGs as data URIs for background-image
and
> > shape-outside seems to be parsed and attached synchronously from style
update
> > for the document they're used by (I've not debugged, based on bot-output).
> > > 
> > > Back to storing on Document, or untangling the synchronous loading?
> > 
> > I'd go back to storing on the document, but just do it in the constructor
> where
> > you put it on the stack.
> > 
> > NthCache cache(document)
> 
> Which doesn't work because Document is a heap-allocated GCed object which
cannot
> contain a raw ptr to a stack allocated GCed object, the clang oilpan plugin
> tells me. :-(
> 

It would be unsafe to accept that as-is, it clearly cannot infer the discipline
for setting&clearing the reference, so it makes sense to not allow it.

However, it is safe (I think..haven't seen your forthcoming patchset), so in
that case we clearly document the reason why it should be considered safe & be
allowed.

Something like,

  // It is safe to keep a raw, untraced pointer to this stack-allocated
  // cache object: it is set upon the cache object being allocated on
  // the stack and cleared upon leaving its allocated scope. Hence it
  // is acceptable not to trace it -- should a conservative GC occur,
  // the cache object's references will be traced by a stack walk.
  GC_PLUGIN_IGNORE("461878")
  NthIndexCache* m_nthIndexCache;

i.e., the GC_PLUGIN_IGNORE("") annotation turns off the GC plugin's attention.
It is to be accompanied with an explanation of why we are escaping the static
checks made by the oilpan plugin. And a relevant bug reference.

hth; (the lazy scheme ps#11 looks quite alright to me, this seems to be
overoptimizing a bit.)

[rune, 2015-04-07 07:22:28]

On 2015/04/04 18:29:06, sof wrote:

> Something like,

>   GC_PLUGIN_IGNORE("461878")
>   NthIndexCache* m_nthIndexCache;

Done.

[sof, 2015-04-07 07:45:51]

lgtm

[rune, 2015-04-07 08:02:13]

The CQ bit was checked by rune@opera.com

[rune, 2015-04-07 08:02:14]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1023393002/#ps280001
(title: "Store NthCacheIndex on Document")

[commit-bot: I haz the power, 2015-04-07 08:02:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023393002/280001

[commit-bot: I haz the power, 2015-04-07 08:07:30]

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

[commit-bot: I haz the power, 2015-04-07 08:07:31]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/50738)

[rune, 2015-04-07 08:25:49]

The CQ bit was checked by rune@opera.com

[rune, 2015-04-07 08:25:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1023393002/#ps300001
(title: "Rebased")

[commit-bot: I haz the power, 2015-04-07 08:25:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023393002/300001

[commit-bot: I haz the power, 2015-04-07 09:16:15]

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

[commit-bot: I haz the power, 2015-04-07 09:16:16]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)

[rune, 2015-04-07 12:33:14]

The CQ bit was checked by rune@opera.com

[rune, 2015-04-07 12:33:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1023393002/#ps320001
(title: "Missing initializer and incorrect assert.")

[commit-bot: I haz the power, 2015-04-07 12:33:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023393002/320001

[commit-bot: I haz the power, 2015-04-07 14:00:46]

Committed patchset #17 (id:320001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193268