Closed Bug 396796 Opened 17 years ago Closed 16 years ago

2% Tdhtml regression from bug 395397 on 9/19/2007

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jaas, Assigned: smichaud)

References

Details

(Keywords: perf, regression)

Attachments

(1 file)

Possible fix [checked in 2007-09-24 17:01]
9.11 KB, patch
jaas
: review+
mark
: review+
roc
: superreview+
damons
: approval1.9+
Details | Diff | Splinter Review 
There was a 2% Tdhtml regression from bug 395397 on 5/19/2007.
I suspect if this is anything other than fuzz then it is something we're going to have to live with, fewer dropped events in an animation or something else similar to comments 36-40 in bug 375436.
Assignee: joshmoz → smichaud
I will see if there's anything I can do about this ... though like
Josh I suspect there probably isn't.

By the way, Colin told me (early on, just after the patch landed) that
"trace_malloc allocs have spiked".  Was this consistent (did it
continue happening in later tests)?  And if so does it suggest to
anyone where I might look to make the Tdhtml results better?

(In reply to comment #2)
> I will see if there's anything I can do about this ... though like
> Josh I suspect there probably isn't.
> 
> By the way, Colin told me (early on, just after the patch landed) that
> "trace_malloc allocs have spiked".  Was this consistent (did it
> continue happening in later tests)?  And if so does it suggest to
> anyone where I might look to make the Tdhtml results better?

Yeah, that's a huge jump.

http://build-graphs.mozilla.org/graph/query.cgi?testname=trace_malloc_allocs&units=count&tbox=bm-xserve11.build.mozilla.org&autoscale=1&days=7&avg=1&showpoint=2007:09:19:16:39:12,573981
Summary: 2% Tdhtml regression from bug 395397 on 5/19/2007 → 2% Tdhtml regression from bug 395397 on 9/19/2007
What exactly do the trace_malloc_allocs numbers mean?
(In reply to comment #4)
> What exactly do the trace_malloc_allocs numbers mean?

Number of calls to malloc during the test. But these look wrong.

It says we're calling XRE_GetFileFromPath and JSD_ functions a bunch. That doesn't make much sense to me.

Maybe you should try running the trace_malloc_alloc tests on a
non-optimized build, or at least on a build that contains debugging
symbols ("-g -O2" will make an optimized build with debugging
symbols).  I know from personal experience that stack traces from
builds without debug symbols are often corrupt.

(In reply to comment #6)
> Maybe you should try running the trace_malloc_alloc tests 

Not so fast. :)

It's your regression, so you get to investigate. That said, I'm using a build with this patch applied, and it does feel slicker.

> It's your regression, so you get to investigate.

I don't know how to run the trace_malloc_alloc tests (or what they
are).  At least tell me where I can RTFM :-)

Here's a patch that (in my tests) gets rid of the spike in number of
calls to malloc, and also (in at least some of my tests) gets rid of
the Dhtml performance regression.

The figures from the trace_malloc tests didn't help -- compiling in
debug symbols doesn't appear to make any difference.

But I got some results when I fell back to old-fashioned trial and
error:

It turns out that (due to a misreading of some very scanty and obscure
Apple docs) I was starving OS timer events.  This should now be fixed.

I also found that it helps to reduce kHadMoreEventsCountMax from 10
to 3.
Attachment #281847 - Flags: review?(joshmoz)
Attachment #281847 - Flags: review?(mark)
Comment on attachment 281847 [details] [diff] [review]
Possible fix [checked in 2007-09-24 17:01]

OK, we can try this.

We're not using NSRunLoop acceptInputForMode:beforeDate: at all now?
Attachment #281847 - Flags: review?(mark) → review+
Comment on attachment 281847 [details] [diff] [review]
Possible fix [checked in 2007-09-24 17:01]

+      if (nextEvent = [NSApp nextEventMatchingMask:NSAnyEventMask

I prefer assignments right before the test just to be more explicit.
Attachment #281847 - Flags: superreview?(roc)
Attachment #281847 - Flags: review?(joshmoz)
Attachment #281847 - Flags: review+
> We're not using NSRunLoop acceptInputForMode:beforeDate: at all now?

Not after this change (I just checked).

> I prefer assignments right before the test just to be more explicit.

You mean I should assign a value to nextEvent and then test it?

I'm afraid that will make this code (already quite complex) seriously
uglier, and harder to read.

skip it then, its not a big deal and really depends on the case
Comment on attachment 281847 [details] [diff] [review]
Possible fix [checked in 2007-09-24 17:01]

Could you check in the change to kHadMoreEventsCountMax separately, after some delay, so we can distinguish the perf effects of the two parts?
Attachment #281847 - Flags: superreview?(roc)
Attachment #281847 - Flags: superreview+
Attachment #281847 - Flags: approval1.9+
> Could you check in the change to kHadMoreEventsCountMax separately,
> after some delay, so we can distinguish the perf effects of the two
> parts?

No problem.

Would you like me to land the changed nsAppShell.mm now (which
includes everything but the change to kHadMoreEventsCountMax), and the
change to nsAppShell.h (which includes only the chanage to
kHadMoreEventsCountMax) somewhat later?

How much later should the second change be?

First part of patch (nsAppShell.mm) landed on trunk.

The first part of my patch doesn't seem to have made any difference to
performance whatsoever ... which is very puzzling, and doesn't agree
with the results I got in my own tests.

I wonder if there's something wrong with the test machines.  Do they
need to rebuild from scratch to give truly accurate results?

Anyway, I'm about to land the second part of my patch (nsAppShell.h).

Second part of patch (nsAppShell.h) landed on trunk.

The second part of my patch does seem to have made a 6-7% cut in the
number of calls to malloc.  This is less than I saw in my own tests
(of the two parts together) ... but at least it's something.

I need to redo some of my tests.  But it does seem that (on the
"official" test machines) the performance contribution of the first
part of my patch is negligible.  At the very least I'll need to change
my comments.

I also have some new ideas I want to try.  So I'll probably post a new
patch later today.

I'm not sure I'll be able to do anything about the Dhtml performance
numbers.  In my own tests I saw the 2% regression disappear on my PPC
Mac, but stay about the same on my Intel Mac.  Why there should be a
difference I don't know ... but at least in this case my results are
consistent with those of the official test machines (which I assume
are all Intel boxes).

I also notice that the Dhtml performance numbers often "spontaneously"
go up or down by 1%-2%.  So I'm not sure how significant a 2% Dhtml
performance regression (or improvement) really is.

I just landed a comments-only patch that corrects my comments from
attachment 281847 [details] [diff] [review].

I no longer give any credit for the patch's performance gain to the
code (in ProcessNextNativeEvent()) that special-cases timer events.
And I do give credit (but only for the reduction in the number of
malloc calls) to reducing kHadMoreEventsCountMax from '10' to '3'.

I tried out an optimization (in ProcessNextNativeEvent()) that I'd
thought of.  But though it did show a small improvement (a 2%-4%
further reduction in the number of calls to malloc), I didn't think
this was worth the risk of causing other problems.

And judging by past experience, it's likely that the 2%-4% improvement
that I saw on my MacBook Pro would have gotten reduced to 1%-2% on the
"official" test machines.

Is this bug still in process?  The patch looks like it was checked in.
> The patch looks like it was checked in.

Yes, it was (on 2007-09-24 15:31 and 17:01).  And another patch was
checked in later to correct some of the comments (2007-09-26 16:05).

> Is this bug still in process?

But, though the patch did improve performance (it reduced the number
of calls to malloc by 6%-7%), it didn't make any change to Tdhtml
performance figures (at least on the official test machine).  So this
bug should probably be left open.

However, I don't consider it a serious problem (certainly not a
blocker), and I probably won't get to it anytime soon.

Quoting from the previous comment #21:

> I also notice that the Dhtml performance numbers often
> "spontaneously" go up or down by 1%-2%.  So I'm not sure how
> significant a 2% Dhtml performance regression (or improvement)
> really is.

Comment on attachment 281847 [details] [diff] [review]
Possible fix [checked in 2007-09-24 17:01]

Resetting all approval1.9+ flags on bugs that have not been checked in by Oct 22 11:59 PM PDT.  Please re-request approval if needed.
Attachment #281847 - Flags: approval1.9+
Comment on attachment 281847 [details] [diff] [review]
Possible fix [checked in 2007-09-24 17:01]

Landed by smichaud on 2007-09-24 17:01.
Attachment #281847 - Flags: approval1.9?
Comment on attachment 281847 [details] [diff] [review]
Possible fix [checked in 2007-09-24 17:01]

Reset approval flag to + as it was already checked in.
Attachment #281847 - Flags: approval1.9? → approval1.9+
Attachment #281847 - Attachment description: Possible fix → Possible fix [checked in 2007-09-24 17:01]
I don't think we're going to follow up on this any more, the most significant issues here have been resolved. Marking FIXED.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: