Closed Bug 213943 Opened 20 years ago Closed 10 years ago

Tracking of overhead involved in accessing DOM objects

Categories

(Core Graveyard :: Tracking, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: bzbarsky, Unassigned)

References

(Depends on 1 open bug)

Details

(Keywords: meta, perf, testcase)

Attachments

(2 files)

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.
Attached file The testcase
On that testcase, we are interested in whatever goes on outside
nsHTMLDivElement::SetAttribute
Depends on: 213946
Keywords: meta, perf, testcase
Blocks: 118933
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.
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.
Depends on: 213813
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.
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).
> 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.
Hmm... Good point.  Didn't jst do some perf tests and decide that plhash is 
faster than pldhash here?  Also, see bug 166013 
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.)
Blocks: 203448
Blocks: 201082
Can we take some of the findings here into 1.8 ?
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.
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.
> 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.  ;)
Depends on: 297959
Assignee: nobody → nobody
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
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.
Using the testcase, average of 10 runs:
Nightly 27 - 75ms
Chrome 29 - 200ms
IE 10 - 642ms
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
Yeah, WebIDL bindings have totally killed this dead as initially filed.  ;)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.