Last Comment Bug 873640 - Read-ahead not actually being used on omni.ja
: Read-ahead not actually being used on omni.ja
Status: RESOLVED FIXED
[Snappy:P2]
: regression
Product: Core
Classification: Components
Component: Networking: JAR (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks: start-faster 810151 852961 1195331
  Show dependency treegraph
 
Reported: 2013-05-17 13:22 PDT by Vladan Djeric (:vladan)
Modified: 2015-08-17 07:58 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
Read-ahead the entire file (969 bytes, patch)
2013-05-17 13:22 PDT, Vladan Djeric (:vladan)
no flags Details | Diff | Review
Fix and cleanup profiledbuild profile script invocation (17.45 KB, patch)
2013-05-17 23:22 PDT, Mike Hommey [:glandium]
ted: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Vladan Djeric (:vladan) 2013-05-17 13:22:05 PDT
Created attachment 751194 [details] [diff] [review]
Read-ahead the entire file

I noticed xperf reporting that only half of omni.ja was being read on startup on Win7, despite read-ahead having been added in bug 810151. I then stepped through the code and found that the "readaheadLength" variable is 0 for all the omni.ja's that ship with Nightly and Aurora.
Comment 1 Aaron Klotz [:aklotz] (Not reviewing until July, ping on IRC if urgent) 2013-05-17 13:29:14 PDT
Hmm... this was working when I first checked things in.

I'll verify this, but if that value is 0 then it is probably not being set correctly when omni.ja is generated in a PGO build.
Comment 2 Mike Hommey [:glandium] 2013-05-17 13:31:53 PDT
Indeed, this is supposed to be filled.
Comment 3 Mike Hommey [:glandium] 2013-05-17 13:40:55 PDT
I'll look into it.
Comment 4 Mike Hommey [:glandium] 2013-05-17 13:48:40 PDT
This is likely a regression from bug 840094
Comment 5 Mike Hommey [:glandium] 2013-05-17 13:58:29 PDT
The cause is actually very likely bug 852961.
Comment 6 Mike Hommey [:glandium] 2013-05-17 14:03:42 PDT
Yeah, this is it, bug 852961 changed the directory under which the profile script is run, which broke the OBJDIR and JARLOG_FILE paths given to it from client.mk, because they are relative.
Comment 7 Mike Hommey [:glandium] 2013-05-17 23:22:16 PDT
Created attachment 751307 [details] [diff] [review]
Fix and cleanup profiledbuild profile script invocation

This does a few things:
- Remove profile_pageloader.*. These were added in bug 237239 and haven't been used since bug 418772 landed (profileserver.py)
- Remove OBJDIR variable from the script invocation, as it was a leftover from using profile_pageloader.pl
- Set JARLOG_FILE relative to the objdir, which is where the profileserver.py script is invoked from
- Replace the PROFILE_GEN_SCRIPT invocation with the raw EXTRA_TEST_ARGS=10 $(MAKE) -C $(PGO_OBJDIR) pgo-profile-run ; this also means we're giving that 10 on all platforms, now, contrary to before, and I don't see any reason there should be a difference between platforms for that. And now that we have that make rule ro run the script, there's no need to use a PROFILE_GEN_SCRIPT variable to set it.
- Remove PROFILE_GEN_SCRIPT definitions from mozconfigs
Comment 8 Ted Mielczarek [:ted.mielczarek] 2013-05-20 17:12:15 PDT
Comment on attachment 751307 [details] [diff] [review]
Fix and cleanup profiledbuild profile script invocation

Review of attachment 751307 [details] [diff] [review]:
-----------------------------------------------------------------

Can you clean up the documentation after you make this change?
https://developer.mozilla.org/en-US/docs/Building_with_Profile-Guided_Optimization

I guess this removes the ability to have custom profiling scenarios without patching the code, but I don't think that's a big deal.
Comment 10 Mike Hommey [:glandium] 2013-05-20 23:43:11 PDT
Updated mdn:
https://developer.mozilla.org/en-US/docs/Building_with_Profile-Guided_Optimization
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-05-21 10:44:20 PDT
https://hg.mozilla.org/mozilla-central/rev/d7fa77615273
Comment 12 Mike Hommey [:glandium] 2013-05-21 11:01:25 PDT
Comment on attachment 751307 [details] [diff] [review]
Fix and cleanup profiledbuild profile script invocation

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression from bug 852961
User impact if declined: Startup time regressions
Testing completed (on m-c, etc.): Validated on PGO builds on m-i
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None
Comment 14 Octoploid 2013-06-22 07:22:44 PDT
I've tried a PGO build on Linux today, but it fails:

make[1]: Leaving directory `/var/tmp/moz-build-dir'
rm -f /var/tmp/moz-build-dir/jarlog/en-US.log
MOZ_PGO_INSTRUMENTED=1 JARLOG_FILE=jarlog/en-US.log EXTRA_TEST_ARGS=10 make -C /var/tmp/moz-build-dir pgo-profile-run
make[1]: Entering directory `/var/tmp/moz-build-dir'
make[1]: *** No rule to make target `pgo-profile-run'.  Stop.
make[1]: Leaving directory `/var/tmp/moz-build-dir'
make: *** [profiledbuild] Error 2

I have the following in my .mozconfig:
ac_add_options --enable-profile-guided-optimization
mk_add_options PROFILE_GEN_SCRIPT=/home/markus/run-firefox.sh

and run:
make -f client.mk profiledbuild

Note You need to log in before you can comment on or make changes to this bug.