1382914 - Consider avoiding using a property in nsRange::Register/UnregisterCommonAncestor()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[(no longer active)]

See this profile: https://perfht.ml/2tkMDhy

This has a lot of unneeded allocator ahd hashtable overhead, and setting selections can be super common, it would be nice to do something more efficient, such as storing this in nsINode::nsSlots instead of in a property.  This should still fit in the same jemalloc bucket

Olli, what do you think?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Sounds reasonable, now that normal slots are a bit less heavy.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

And when doing that we should also add some static assertions to ensure the sizeof slots isn't increased without some thought.

Comment 3

[(no longer active)]

Attachment: Store the hashtable of ancestor ranges in the node slots instead of in a property in order to speed up access to it

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8889029 [details] [diff] [review]
Store the hashtable of ancestor ranges in the node slots instead of in a property in order to speed up access to it

>   if (IsCommonAncestorForRangeInSelection()) {
>-    nsRange::RangeHashTable* ranges =
>-      static_cast<nsRange::RangeHashTable*>(GetProperty(nsGkAtoms::range));
>+    auto ranges = GetCommonAncestorRanges();
Don't use 'auto', but the actual type here. It isn't clear to the reader what type 'ranges' is
>+  const nsTHashtable<nsPtrHashKey<nsRange>>* GetCommonAncestorRanges() const
>+  {
>+    if (!HasSlots()) {
>+      return nullptr;
>+    }
>+    auto& ranges = GetExistingSlots()->mCommonAncestorRanges;
>+    return ranges ? &*ranges : nullptr;
>+  }
Please use real types here to make this readable.


>+
>+  nsTHashtable<nsPtrHashKey<nsRange>>* GetCommonAncestorRanges()
>+  {
>+    auto& ranges = GetCommonAncestorRangesPtr();
>+    return ranges ? &*ranges : nullptr;
>+  }
ditto.

>+
>+  mozilla::UniquePtr<nsTHashtable<nsPtrHashKey<nsRange>>>& GetCommonAncestorRangesPtr()
>+  {
A bit weird naming when no pointer is returned, but UniquePtr, but I don't have better suggestions.


>   for (; n; n = GetNextRangeCommonAncestor(n->GetParentNode())) {
>-    RangeHashTable* ranges =
>-      static_cast<RangeHashTable*>(n->GetProperty(nsGkAtoms::range));
>+    auto ranges = n->GetCommonAncestorRanges();
Please use concrete type to make this more readable.
(In my mind auto should be used only when doing a cast, which shows the actual type, or with iterations, but even iterations
can be a bit error prone with auto.)

>-  RangeHashTable* ranges =
>-    static_cast<RangeHashTable*>(aNode->GetProperty(nsGkAtoms::range));
>+  auto& ranges = aNode->GetCommonAncestorRangesPtr();
Real type please

>-  RangeHashTable* ranges =
>-    static_cast<RangeHashTable*>(aNode->GetProperty(nsGkAtoms::range));
>+  auto ranges = aNode->GetCommonAncestorRanges();
real type please

> nsRange::GetRegisteredCommonAncestor()
> {
>   NS_ASSERTION(IsInSelection(),
>                "GetRegisteredCommonAncestor only valid for range in selection");
>   nsINode* ancestor = GetNextRangeCommonAncestor(mStartContainer);
>   while (ancestor) {
>-    RangeHashTable* ranges =
>-      static_cast<RangeHashTable*>(ancestor->GetProperty(nsGkAtoms::range));
>-    if (ranges->GetEntry(this)) {
>+    auto ranges = ancestor->GetCommonAncestorRanges();
real type please

>+    if (ranges && ranges->GetEntry(this)) {
Why we need null check here, but didn't need before?

>   fprintf(out, "Text@%p", static_cast<const void*>(this));
>   fprintf(out, " flags=[%08x]", static_cast<unsigned int>(GetFlags()));
>   if (IsCommonAncestorForRangeInSelection()) {
>-    typedef nsTHashtable<nsPtrHashKey<nsRange> > RangeHashTable;
>-    RangeHashTable* ranges =
>-      static_cast<RangeHashTable*>(GetProperty(nsGkAtoms::range));
>+    auto ranges = GetCommonAncestorRanges();
real type


Trying to sell 'auto' to me? Not going to work ;) We've had security critical bugs because of auto, we have had memory leaks because of auto...

Comment 5

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #4)
> >+    if (ranges && ranges->GetEntry(this)) {
> Why we need null check here, but didn't need before?

I suppose we don't, since the conditions where this will return null are exactly the same as before.

I'll switch away from using auto because I don't feel like starting a religious argument about it, especially since you make it clear that you aren't going to change your mind before I even say anything.  :/

Comment 6

[(no longer active)]

Attachment: Store the hashtable of ancestor ranges in the node slots instead of in a property in order to speed up access to it

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8889545 [details] [diff] [review]
Store the hashtable of ancestor ranges in the node slots instead of in a property in order to speed up access to it

(I am always curious to know why people think use of auto is a good thing.
So far I think only generated code or iterators have had some good reasoning,
and using auto with static_cast is just programmer being lazy, but at least it is clear what the type is there.)


>+  const nsTHashtable<nsPtrHashKey<nsRange>>* GetCommonAncestorRanges() const
>+  {
>+    if (!HasSlots()) {
>+      return nullptr;
>+    }
>+    mozilla::UniquePtr<nsTHashtable<nsPtrHashKey<nsRange>>>& ranges =
>+      GetExistingSlots()->mCommonAncestorRanges;
>+    return ranges ? &*ranges : nullptr;
>+  }
>+
>+  nsTHashtable<nsPtrHashKey<nsRange>>* GetCommonAncestorRanges()
>+  {
>+    mozilla::UniquePtr<nsTHashtable<nsPtrHashKey<nsRange>>>& ranges =
>+      GetCommonAncestorRangesPtr();
>+    return ranges ? &*ranges : nullptr;
>+  }
Why the non-const version calls GetCommonAncestorRangesPtr() which calls Slots() ?
That ends up creating Slots always. This fixed or explained, r+

Do we need return ranges ? &*ranges : nullptr;
Wouldn't return ranges.get(); work? That would be at least easier to read.

Comment 8

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #7)
> (I am always curious to know why people think use of auto is a good thing.
> So far I think only generated code or iterators have had some good reasoning,
> and using auto with static_cast is just programmer being lazy, but at least
> it is clear what the type is there.)

IMO there are cases where its usage is strictly better than not using it by making the code easier to read (for example by removing information that is repeated twice in the same statement such as with |auto foo = static_cast<T>(bar);|) and there are cases where its usage is strictly worse than not using it (where through not being careful you'd end up creating expensive copies of an iterator in a range-based for loop like |for (auto iter : container) doSomething()|) and there is an ocean of cases in between these cases where reasonable people can agree or disagree on its usage.

I don't really have that strong of an opinion on its usage in general, and I can certainly agree that in some cases its usage isn't preferable (including in the first iteration of this patch after re-reading in myself perhaps in nsRange::IsNodeSelected) but I find going through patches and rejecting auto's one by one pretty unreasonable.  Anyway, maybe that's just me.  But at any rate, I wasn't trying to convince you of anything, just moving on...

> >+  const nsTHashtable<nsPtrHashKey<nsRange>>* GetCommonAncestorRanges() const
> >+  {
> >+    if (!HasSlots()) {
> >+      return nullptr;
> >+    }
> >+    mozilla::UniquePtr<nsTHashtable<nsPtrHashKey<nsRange>>>& ranges =
> >+      GetExistingSlots()->mCommonAncestorRanges;
> >+    return ranges ? &*ranges : nullptr;
> >+  }
> >+
> >+  nsTHashtable<nsPtrHashKey<nsRange>>* GetCommonAncestorRanges()
> >+  {
> >+    mozilla::UniquePtr<nsTHashtable<nsPtrHashKey<nsRange>>>& ranges =
> >+      GetCommonAncestorRangesPtr();
> >+    return ranges ? &*ranges : nullptr;
> >+  }
> Why the non-const version calls GetCommonAncestorRangesPtr() which calls
> Slots() ?
> That ends up creating Slots always. This fixed or explained, r+

Precisely because I want the non-const version to create the slots if needed, but the const version can't do that (as the function is const, so it can't modify the object, so it'll return null if there are no slots, and use GetExistingSlots() to access an already existing slots if one is available.)

Note that the const version of this function is only used in the debugging Element::List() and nsTextNode::List() methods and nowhere else.

Is this OK with you?  (Double checking this point before I land)

> Do we need return ranges ? &*ranges : nullptr;
> Wouldn't return ranges.get(); work? That would be at least easier to read.

Sure.  I did it this way to avoid calling .get() on a known null pointer.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #8)
 
> Precisely because I want the non-const version to create the slots if
> needed,
But why? Why you need slots pointing to empty hashtable?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

FWIW, when people started to use auto in Gecko, we had so many major issues, that it became quite clear to me that issues aren't easily caught by patch authors nor reviewers, so, IMO, better to just try to avoid use that error prone feature in cases it doesn't actually improve anything.

Comment 11

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #9)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #8)
>  
> > Precisely because I want the non-const version to create the slots if
> > needed,
> But why? Why you need slots pointing to empty hashtable?

I'm not sure I follow, when would we create slots pointing to an empty hashtable?  As far as I can tell the only case where this can create slots is when called from GetCommonAncestorRangesPtr() from nsRange::RegisterCommonAncestor() where we immediately add something to the hashtable.  Am I missing something?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8889545 [details] [diff] [review]
Store the hashtable of ancestor ranges in the node slots instead of in a property in order to speed up access to it

>+  nsTHashtable<nsPtrHashKey<nsRange>>* GetCommonAncestorRanges()
>+  {
>+    mozilla::UniquePtr<nsTHashtable<nsPtrHashKey<nsRange>>>& ranges =
>+      GetCommonAncestorRangesPtr();
>+    return ranges ? &*ranges : nullptr;
>+  }

Here you call GetCommonAncestorRangesPtr() which creates Slots. But that doesn't guarantee that mCommonAncestorRanges is non-null, so
GetCommonAncestorRanges may still return null.

Comment 13

[(no longer active)]

Talked to Olli on IRC, agreed to drop the GetSlots() call from GetCommonAncestorRanges, and rename it to GetExistingCommonAncestorRanges() to make the distinction with GetCommonAncestorRangesPtr() a bit more clear.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/288c2f3f9dd5
Store the hashtable of ancestor ranges in the node slots instead of in a property in order to speed up access to it; r=smaug

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6e6a6e9bbb
follow-up: Spell GetExistingCommonAncestorRanges() correctly

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/288c2f3f9dd5
https://hg.mozilla.org/mozilla-central/rev/6b6e6a6e9bbb