213943 - Tracking of overhead involved in accessing DOM objects

Status: RESOLVED WORKSFORME

(Core Graveyard :: Tracking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is a bug tracking overhead involved in accessing various DOM objects.  The
testcase is just something that does a no-op access a few hundred thousand times.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: The testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

On that testcase, we are interested in whatever goes on outside
nsHTMLDivElement::SetAttribute

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: jprof profile of one time through that testcase

Comment 4

[David Bradley]

I tried out the FindMember optimization patch in bug 213943. Surprisingly I only
saw a modest gain of 2 to 4 percent. I verified that it was getting cached. The
method being called is the 26th element of the second interface. The first
interface only had one member.

I suspect that the call overhead is only a small portion of the time in this
loop. Unfortunately my system isn't good at running perf tests so I can't
confirm this. It could be that the the difference between looping to the 26th
method isn't that expensive over the hashing, but I would think we'd see a
decent win in that situation.

So the patch does help a bit, but apparently isn't the main source of time spent.

Comment 5

[Boris Zbarsky [:bzbarsky]]

David, 2-4% win on raw time on that testcase means a 4-8% reduction in the 
JS/XPConnect overhead (since such overhead is only 50% of the total time; the 
other 50% is security manager and actually setting the attribute).

Doesn't sound too shabby at all.

Comment 6

[David Bradley]

new, malloc, and free are at the top of the list again for this tests as well.

new seems to be driven by two areas. One is a string conversion between JS and
native. The other is the nsNodeInfoManager::GetNodeInfo, which if you follow it
up you'll get to nsGenericHTMLElement::NormalizeAttrString.

What I find odd is that we're getting an allocation for each call to add a entry
to the hash table via PL_HashTableAdd. I don't know much about the
implementation but that doesn't seem very efficient. Shouldn't the hash table be
block allocating entries in a more efficient manner? Is there a better hash to
use here? At a higher level, nsNodeInfoManager seems to add and remove the
entries for each call in GetNodeInfo and RemoveNodeInfo. So again I'm not sure
why the hash logic has to do allocations, I would expect it to remain a constant
size. 

malloc is driven by duplicating the string in nsDOMClassINfo::GetClassName,
which in turn is generated by the security check. I wonder if it might be
interesting to cache the results from the security check.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Odd.  We should malloc from the hashtable code only if the hashtable is 
overloaded, if I read the code correctly....  Brendan?  dbaron?  Any idea 
what's up there?  In general, btw, I'm ignoring anything happening under 
SetAttr for purposes of this bug; that code is in sore need of modification in 
various ways and I'd rather profile it once said modifications have taken place.

As for GetClassName, see bug 213946 comment 0 (that talks about 
GetClassDescription, but the latter calls the former).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> We should malloc from the hashtable code only if the hashtable is 
> overloaded, if I read the code correctly....

dbradley said PL_HashTableAdd.  Are you reading PL_DHashTableOperate instead?

plhash does have allocEntry / freeEntry callbacks that can be used to use an
arena, but you may be better off using pldhash instead.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm... Good point.  Didn't jst do some perf tests and decide that plhash is 
faster than pldhash here?  Also, see bug 166013 

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

plhash may well be faster (and use more memory, most likely).  But you could
probably make it faster still with custom allocators using an arena, since the
performance seems to be an issue here.

(See http://lxr.mozilla.org/seamonkey/source/tools/trace-malloc/tmreader.c for
an example.)

Comment 11

[Markus Hübner]

Can we take some of the findings here into 1.8 ?

Comment 12

[Boris Zbarsky [:bzbarsky]]

What findings, exactly?  There are no findings in this bug that I can see. 
Please stop spamming bugs with comments like this, Markus.  It's very
disheartening to wake up to a huge stream of pointless bugmail like this
morning.  Your time could be better-spent analyzing the profile and trying to
find ways to improve the code.

Comment 13

[Markus Hübner]

I was refering to comment #10 - this sounds like a promising suggestion. Maybe 
someone has already tried that. I am more a QA guy no coder, especially that 
area of code is the game of a view I think.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I was refering to comment #10 - this sounds like a promising suggestion.

It's a fair amount of work, and could help a tad.  There are certainly bigger
performance fish to fry (security manager, eg).

> I am more a QA guy no coder

That's not immutable.  ;)

Comment 15

[Guilherme Lima]

BZ, is this still a problem?

I tested with today's nightly build and got Average (10runs) :120ms
On IE8 I got Average (10runs) :4031ms
And Google Chrome 8 I got Average (10runs) :156ms

Comment 16

[Boris Zbarsky [:bzbarsky]]

This is a tracking bug...  The testcase was just an example.

That said, we're definitely much better on this overhead, and we have some more ideas for reducing it further.  See bug 622298.  So it's fine to resolve this one worskforme or whatever, probably.

Comment 17

[Guilherme Lima]

Using the testcase, average of 10 runs:
Nightly 27 - 75ms
Chrome 29 - 200ms
IE 10 - 642ms

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yeah, WebIDL bindings have totally killed this dead as initially filed.  ;)