Profile: -P adds 3% to startup time

VERIFIED FIXED in mozilla0.9.9

Status

defect
P1
normal
VERIFIED FIXED
18 years ago
3 years ago

People

(Reporter: mcafee, Assigned: tetsuroy)

Tracking

({perf})

Trunk
mozilla0.9.9
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

18 years ago
Find a build with one profile, and (redundantly) specify
the profile with -P <profilename>, this bumped two of my
performance tinderbox startup times up by about 3%.
20.18 secs to 20.70 secs, something like that.

What's going on here?  Just profile service overhead?
Reporter

Updated

18 years ago
Keywords: perf
OS: Windows 2000 → All
Hardware: PC → All
When you use -P, the code path is slightly shorter than when you don't. Using -P
bypasses nsProfile::LoadDefaultProfileDir().

It may be this code which went in recently:
http://lxr.mozilla.org/mozilla/source/profile/src/nsProfile.cpp#842
I doubt it though because, on Windows & Unix, reading the registry uses
NS_NewUnicodeLocalFile which uses uconv services (a huge problem we're working
to solve in the nsIFile impl) so that code should already be loaded and the code
I mentioned should have very little overhead.

Roy - In case it is that, your code which converts to the platform charset
should just check the incoming string using IsASCII before resorting to uconv
services. If it already is ASCII, a simple cheap conversion could be done.
 
Sorry, that link was wrong. It was the similar looking code at
http://lxr.mozilla.org/mozilla/source/profile/src/nsProfile.cpp#777
*** Bug 112871 has been marked as a duplicate of this bug. ***
Assignee

Comment 4

18 years ago
conrad: I can put the patch together to utilize the IsASCII().

However, as you noted in comment #1, it probably doesn't 
improve 3% of start-up.

Updated

18 years ago
Blocks: 7251
Assignee

Comment 5

18 years ago
Assignee

Comment 6

18 years ago
I did the Start-up performance test result
and here it is:

# run | without patch      | with patch
------+--------------------+---------------
1     | 10.422             | 10.422
2     | 10.437             | 10.437
3     | 10.438             | 10.406
4     | 10.422             | 10.406
5     | 10.438             | 10.390
6     | 10.422             | 10.421
7     | 10.422             | 10.406
8     | 10.422             | 10.422
------+--------------------+---------------
ave   | 10.4278 sec        | 10.4137 sec

The result is 3.3% better with the patch.
Assignee

Comment 7

18 years ago
Conrad: I'll check in the patch if you'd like. Let me know.
Comment on attachment 60883 [details] [diff] [review]
Check for ASCII before uconv service

r=ccarlen. Thanks Roy - I'll give it to you.
Attachment #60883 - Flags: review+
-> Roy
Assignee: ccarlen → yokoyama
Assignee

Updated

18 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.7
Assignee

Comment 10

18 years ago
brendan: can you super review?
Comment on attachment 60883 [details] [diff] [review]
Check for ASCII before uconv service

Existing code, I realize, but it's silly to test (cmdResult) after you've
established that NS_SUCCEEDED(rv) from a string getter that computed cmdResult.

Otherwise, sr=brendan@mozilla.org, although any more common code and I would
rather see a common subroutine for -P and -CreateProfile to use.

/be
Attachment #60883 - Flags: superreview+
Assignee

Comment 12

18 years ago
brendan: I think we should leave the if (cmdResult) check
because we still receive NS_SUCCEEDED(rv) for an empty cmd string.

-- cc'ing conrad
Roy, my point is that you don't need *both* NS_SUCCEEDED(rv) and the
cmdResult-converted-to-null test.  You don't propagate rv, so just get rid of
the rv assignment and the NS_SUCCEEDED(rv) test.

/be
Assignee

Comment 14

18 years ago
Attachment #60883 - Attachment is obsolete: true
Thanks, my sr= stands.

/be
Assignee

Updated

18 years ago
Whiteboard: Ready to checkin

Comment 16

18 years ago
>ave   | 10.4278 sec        | 10.4137 sec
>The result is 3.3% better with the patch.
??? where is the 3.3% better with the patch?
If with the path is 3.3% better, than we should see 

10.4278 * (1.0 - 0.033) = 10.478 * 0.967 = 10.0836826

10.4137 is only about 0.13 % better than 
10.4278 ( (10.4278-10.4137)/10.4278 * 100% )
Assignee

Comment 17

18 years ago
Sigh, I thought I used the same equation to determine the average.
(I *always* double check the result before I post on the web before
embrass myself in front of public eyes.)
I am pretty sure I got something 3.3xxxx% improvement before; 
but I guess I was little excited.

ftang: So what do you want me to do with the patch? 
       Is it worth check it in?  I think it's worth it.
I think it's a good patch. Question is: are the performance measurements
repeatable to within 3%.

Comment 19

18 years ago
>ftang: So what do you want me to do with the patch? 
>Is it worth check it in?  I think it's worth it.
The question is, does your patch really fix the target issue? If so, check it 
in, if not, don't do so. While the patch have no problem in general, it does 
make the code a little bit complex then before. We should check in if it DOES 
help performacnce. 
Assignee

Comment 20

18 years ago
Conrad: do you have any other ideas for the cause of the slowness?
I would be more than happy to help you out. Just let me know.
Whiteboard: Ready to checkin
Assignee

Comment 21

18 years ago
Conrad: I'll assign this back to you.  I think I am done with my share.
Assignee: yokoyama → ccarlen
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.7 → ---
What is the repeatability of this test which detected this? If it's not within
+-3%, this should be closed. In reading through the code, using -P should, if
anything, be faster (not significantly though).
Closing - see comment #22.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → WORKSFORME

Comment 24

18 years ago
Verified 
Status: RESOLVED → VERIFIED
Reporter

Comment 25

18 years ago
> What is the repeatability of this test which detected this? If it's not within
> +-3%, this should be closed. In reading through the code, using -P should, if
> anything, be faster (not significantly though).
This looks like a really weak justification to close this bug.
Am I missing something, or should we test this to determine
how repeatable the speedup is instead of "looking at it".

Reopening.  Either we should test this and get some real numbers
here, or come up with a better reason to close this.
Status: VERIFIED → REOPENED
Resolution: WORKSFORME → ---
  Once you determined that one code path was 3% slower than another, "looking at
it" is how to determine why. I found that some additional code was added to the
slower code path, and Roy made a patch for that, though it cut a very small
percentage off of the runtime.
  That the two paths of execution are within 3% of each other in cost does not
make it a bug. If the same code slowed down by 3% suddenly, that's a different
story. It was also closed because the question in comment 22 was not answered.
  As it is, I think Roy's patch should be checked in as it does cut *some*
amount of time.

Comment 27

18 years ago
So, dp is clawing and scratching for 1% wins. Now that we've shaken out our test
harness, maybe we could re-collect numbers? Cathleen, you've been doing some
startup testing: have you noticed any difference?

Comment 28

18 years ago
I tried on my win98 machine 200mHz 64MB, it's hard to tell ...

here are the timings, for warm startup

     w/ -P    w/o -P
    -------  ---------
     8450      7860    
     8680      8430
     7830      8760
     8380      7950

avg  8335      8250 

about 1.03% increase.

mcafee is going to tweak one of the tinderbox test to see if we can confirm the
increase, while tree is closed for 0.9.8

Comment 29

18 years ago
I also want to add, people are really chasing after those 0.5% perf wins, and
fixing them.  There aren't any big fish to fry at this stage, so if we do find
the increase is real, lets try to figure this out.  

- thanks!!
Reporter

Comment 31

18 years ago
> That the two paths of execution are within 3% of each other in cost does not
> make it a bug. If the same code slowed down by 3% suddenly, that's a different
> story. It was also closed because the question in comment 22 was not answered.

For the record, I disagree with this statement.  A 3% finding is indeed a
performance problem, and we file bugs to work on these things.

Your question about repeatability re: comment #22 is valid, but
not-having-an-answer does not justify closing this.  If you can't do the
timings, ask for help next time.
--

I don't see any changes between -P and no -P, so I think we have
solved the problem for now.  Marking fixed.
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
Roy's patch was never checked in, and it does appear to be an improvement. Maybe
it stopped making a difference if something new has triggered the early use of
uconv, but that might get fixed and we'll want this ready for it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think Roy's patch should be checked in so back to him. It did some amount of
good and, now that nsLocalFile is not using uconv for unicode path conversion,
it may do even more good.
Assignee: ccarlen → yokoyama
Status: REOPENED → NEW
Assignee

Comment 34

18 years ago
I think any amount of perf improvement is a winner.
I accept this bug and I will checkin my patch.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Assignee

Comment 35

18 years ago
Fix checked in.
From comment #31; I am marking this bug as FIXED.
Please reopen if disgree
Status: ASSIGNED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 36

18 years ago
Verified code fix
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.