Last Comment Bug 887824 - add more startup timeline events to fennec
: add more startup timeline events to fennec
Status: NEW
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 807322
  Show dependency treegraph
 
Reported: 2013-06-27 07:56 PDT by Nathan Froyd [:froydnj]
Modified: 2013-07-11 14:01 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reflect StartupTimeline's enums into Java code (7.22 KB, patch)
2013-06-27 07:58 PDT, Nathan Froyd [:froydnj]
blassey.bugs: review-
mh+mozilla: review-
Details | Diff | Splinter Review
add telemetry timestamps to android's browser.js:BrowserApp.startup (2.24 KB, patch)
2013-06-27 07:58 PDT, Nathan Froyd [:froydnj]
mark.finkle: review+
Details | Diff | Splinter Review
add startupTimelineRecordStamp to GeckoAppShell (4.54 KB, patch)
2013-06-27 07:59 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
add several startup timeline event timestamps to fennec (8.17 KB, patch)
2013-06-27 08:00 PDT, Nathan Froyd [:froydnj]
blassey.bugs: review+
Details | Diff | Splinter Review
add event for the first chrome URI loaded (2.84 KB, patch)
2013-06-27 08:01 PDT, Nathan Froyd [:froydnj]
bugs: review+
Details | Diff | Splinter Review
part 2 - add startupTimelineRecordStamp to GeckoAppShell (5.46 KB, patch)
2013-06-27 08:14 PDT, Nathan Froyd [:froydnj]
blassey.bugs: review+
Details | Diff | Splinter Review
part 1a - change startup events to be generated from a json file (8.67 KB, patch)
2013-07-02 17:36 PDT, Nathan Froyd [:froydnj]
mh+mozilla: review+
Details | Diff | Splinter Review
part 1b - reflect startup events into java (3.91 KB, patch)
2013-07-02 17:37 PDT, Nathan Froyd [:froydnj]
blassey.bugs: review+
Details | Diff | Splinter Review
part 1a - change startup events to be generated from a json file (8.77 KB, patch)
2013-07-03 08:31 PDT, Nathan Froyd [:froydnj]
mh+mozilla: review+
gps: feedback+
Details | Diff | Splinter Review

Description Nathan Froyd [:froydnj] 2013-06-27 07:56:28 PDT
Currently the only extra insight we have into what's going on during Fennec
startup is the linkerInitialized and librariesLoaded timestamps.  We can do
better.  In addition to helping developers understand the flow of things,
these timestamps will also be sent to telemetry to help us understand what
people are seeing in the wild.
Comment 1 Nathan Froyd [:froydnj] 2013-06-27 07:58:09 PDT
Created attachment 768326 [details] [diff] [review]
reflect StartupTimeline's enums into Java code

First order of business: we need the StartupTimeline enums.  I could have
hardcoded these into the Java code, but it seemed better to auto-generate
them from the existing .h file.  Preprocessor.py won't work because it
doesn't really understand macros, so we have to use the C preprocessor.

Flagging for mobile and build review.
Comment 2 Nathan Froyd [:froydnj] 2013-06-27 07:58:56 PDT
Created attachment 768327 [details] [diff] [review]
add telemetry timestamps to android's browser.js:BrowserApp.startup

Seems helpful to know how long our javascript initialization is taking.
Comment 3 Nathan Froyd [:froydnj] 2013-06-27 07:59:31 PDT
Created attachment 768328 [details] [diff] [review]
add startupTimelineRecordStamp to GeckoAppShell

We need to be able to communicate timestamps to Gecko through JNI.
Comment 4 Nathan Froyd [:froydnj] 2013-06-27 08:00:18 PDT
Created attachment 768330 [details] [diff] [review]
add several startup timeline event timestamps to fennec

Several of the most useful timestamps I found from performance debugging.
We buffer up events until we know Gecko is running.
Comment 5 Nathan Froyd [:froydnj] 2013-06-27 08:01:18 PDT
Created attachment 768331 [details] [diff] [review]
add event for the first chrome URI loaded

This is an optional event, but one that I found helpful in understanding what
was going on.  It's very similar to |createTopLevelWindow|, so I can understand
if it's not desired.
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2013-06-27 08:12:47 PDT
Comment on attachment 768331 [details] [diff] [review]
add event for the first chrome URI loaded

Perhaps assert that mItemType is typeChrome in the else.

This code isn't hot at all, so the optimization for about:blank looks a bit odd,
but at least good to have some explanation.
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2013-06-27 08:12:51 PDT
Comment on attachment 768326 [details] [diff] [review]
reflect StartupTimeline's enums into Java code

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

::: mobile/android/base/Makefile.in
@@ +1297,5 @@
>  		cat $< | sed s,@APPNUM@,$$n,g >> $@ ; \
>  	done
>  
> +StartupTimelineEvent.java: StartupTimelineEvent.java.in $(topsrcdir)/toolkit/components/startup/StartupTimeline.h
> +	$(CPP) $(DEFINES) $(ACDEFINES) -I$(topsrcdir)/toolkit/components/startup $< | sed -e '/^#/d' > $@

is this something you can do with the standard preprocessor we use for other preprocessed java files?
Comment 8 Nathan Froyd [:froydnj] 2013-06-27 08:14:41 PDT
Created attachment 768340 [details] [diff] [review]
part 2 - add startupTimelineRecordStamp to GeckoAppShell

Here, let's try attaching something that compiles after furious rebasing.
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2013-06-27 08:18:56 PDT
Comment on attachment 768327 [details] [diff] [review]
add telemetry timestamps to android's browser.js:BrowserApp.startup

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

Finkle would be a better reviewer
Comment 10 Nathan Froyd [:froydnj] 2013-06-27 18:13:59 PDT
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #7)
> Comment on attachment 768326 [details] [diff] [review]
> reflect StartupTimeline's enums into Java code
> 
> Review of attachment 768326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Makefile.in
> @@ +1297,5 @@
> >  		cat $< | sed s,@APPNUM@,$$n,g >> $@ ; \
> >  	done
> >  
> > +StartupTimelineEvent.java: StartupTimelineEvent.java.in $(topsrcdir)/toolkit/components/startup/StartupTimeline.h
> > +	$(CPP) $(DEFINES) $(ACDEFINES) -I$(topsrcdir)/toolkit/components/startup $< | sed -e '/^#/d' > $@
> 
> is this something you can do with the standard preprocessor we use for other
> preprocessed java files?

Unfortunately not; AIUI, the standard preprocessor doesn't understand C macros with arguments and/or token concatenation in macros.  And StartupTimelineEvent.java.in relies on both of those things.
Comment 11 Nathan Froyd [:froydnj] 2013-06-27 18:15:17 PDT
I told Taras that I'd post startup timeline numbers for eideticker nytimes-load with these events included, but my development machine's primary hard drive started failing, so I'll have to transfer everything to another machine.  I'll get those numbers out tomorrow.
Comment 12 Mike Hommey [:glandium] 2013-06-27 19:08:13 PDT
Comment on attachment 768326 [details] [diff] [review]
reflect StartupTimeline's enums into Java code

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

::: mobile/android/base/Makefile.in
@@ +1297,5 @@
>  		cat $< | sed s,@APPNUM@,$$n,g >> $@ ; \
>  	done
>  
> +StartupTimelineEvent.java: StartupTimelineEvent.java.in $(topsrcdir)/toolkit/components/startup/StartupTimeline.h
> +	$(CPP) $(DEFINES) $(ACDEFINES) -I$(topsrcdir)/toolkit/components/startup $< | sed -e '/^#/d' > $@

I think it would be nicer to use a json file with a script that would generate StartupTimelineEvent.java completely, as well as the corresponding StartupTimeline.h bit, so that there's no duplication.
Comment 13 Mike Hommey [:glandium] 2013-06-27 22:19:41 PDT
Comment on attachment 768330 [details] [diff] [review]
add several startup timeline event timestamps to fennec

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

::: mobile/android/base/GeckoAppShell.java
@@ +349,5 @@
>          } catch (NoSuchElementException e) {}
>      }
>  
> +    public static void startupTimelineStamp(int event) {
> +        long timestamp = System.nanoTime();

You should probably not take timestamps from java, but natively from C, so that you ensure you're using the same clock.
Comment 14 Mike Hommey [:glandium] 2013-06-27 23:31:04 PDT
For the record, these are the values I get on a Nexus S:
-  278 browserAppOnCreateStart
-  845 browserAppOnCreateEnd
- 1326 geckoThreadCreated
- 1460 linkerInitialized
- 2369 librariesLoaded
- 2415 main
- 3127 startupCrashDetectionBegin
- 3203 AMI_startup_begin
- 3355 XPI_startup_begin
- 3429 XPI_bootstrap_addons_begin
- 3429 XPI_bootstrap_addons_end
- 3429 XPI_startup_end
- 3430 AMI_startup_begin
- 3620 firstLoadChromeURI
- 3677 createTopLevelWindow
- 4331 androidBrowserStartupStart
- 4611 androidBrowserStartupEnd
- 4701 sessionRestored
- 4871 firstLoadURI
Comment 15 Mike Hommey [:glandium] 2013-06-28 01:28:06 PDT
It takes a further 600ms to get from firstLoadURI to http://hg.mozilla.org/mozilla-central/file/8e3a124c9c1a/netwerk/protocol/http/nsHttpTransaction.cpp#l273 (connected on wifi on a good connection, loading www.mozilla.org.), part of which is loading softokn3, freebl and nssckbi (even when not loading an https url)
Comment 16 Mike Hommey [:glandium] 2013-06-28 02:00:45 PDT
With most of our I/O eliminated (libs and profile in tmpfs):
-  212 browserAppOnCreateStart
-  758 browserAppOnCreateEnd
- 1184 geckoThreadCreated
- 1307 linkerInitialized
- 1520 librariesLoaded
- 1541 main
- 1923 startupCrashDetectionBegin
- 2001 AMI_startup_begin
- 2159 XPI_startup_begin
- 2247 XPI_bootstrap_addons_begin
- 2247 XPI_bootstrap_addons_end
- 2247 XPI_startup_end
- 2247 AMI_startup_end
- 2480 firstLoadChromeURI
- 2517 createTopLevelWindow
- 3046 androidBrowserStartupStart
- 3353 androidBrowserStartupEnd
- 3442 sessionRestored
- 3618 FirstLoadURI

What's interesting in those numbers is not so much how much faster this got, it's how many intervals didn't change significantly (values are previous event to named event, first column is without tmpfs, second column with):
- 278 212 browserAppOnCreateStart
- 567 546 browserAppOnCreateEnd
- 481 426 geckoThreadCreated
- 134 123 linkerInitialized
- 909 213 librariesLoaded
-  46  21 main
- 712 382 startupCrashDetectionBegin
-  76  78 AMI_startup_begin
- 152 158 XPI_startup_begin
-  74  88 XPI_bootstrap_addons_begin
-   0   0 XPI_bootstrap_addons_end
-   0   0 XPI_startup_end
-   1   0 AMI_startup_begin
- 190 233 firstLoadChromeURI
-  57  37 createTopLevelWindow
- 654 529 androidBrowserStartupStart
- 280 307 androidBrowserStartupEnd
-  90  89 sessionRestored
- 170 176 firstLoadURI

Considering the noise in the measures, only librariesLoaded and startupCrashDetectionBegin changed. librariesLoaded is expected. Whatever is done betwen main and startupCrashDetectionBegin is half I/O bound. But everything else is essentially CPU bound.
Comment 17 Nathan Froyd [:froydnj] 2013-06-28 05:20:55 PDT
(In reply to Mike Hommey [:glandium] from comment #12)
> ::: mobile/android/base/Makefile.in
> @@ +1297,5 @@
> >  		cat $< | sed s,@APPNUM@,$$n,g >> $@ ; \
> >  	done
> >  
> > +StartupTimelineEvent.java: StartupTimelineEvent.java.in $(topsrcdir)/toolkit/components/startup/StartupTimeline.h
> > +	$(CPP) $(DEFINES) $(ACDEFINES) -I$(topsrcdir)/toolkit/components/startup $< | sed -e '/^#/d' > $@
> 
> I think it would be nicer to use a json file with a script that would
> generate StartupTimelineEvent.java completely, as well as the corresponding
> StartupTimeline.h bit, so that there's no duplication.

By duplication, you mean the need for private and public fields?

(In reply to Mike Hommey [:glandium] from comment #13)
> ::: mobile/android/base/GeckoAppShell.java
> @@ +349,5 @@
> >          } catch (NoSuchElementException e) {}
> >      }
> >  
> > +    public static void startupTimelineStamp(int event) {
> > +        long timestamp = System.nanoTime();
> 
> You should probably not take timestamps from java, but natively from C, so
> that you ensure you're using the same clock.

I would like to do that, but I need some way of taking timestamps from Java for events that happen before libxul is loaded.  I suppose post-libxul load, we can take timestamps directly in C, but I'm not sure that it's worth it.
Comment 18 Mike Hommey [:glandium] 2013-06-28 05:24:55 PDT
(In reply to Nathan Froyd (:froydnj) from comment #17)
> > I think it would be nicer to use a json file with a script that would
> > generate StartupTimelineEvent.java completely, as well as the corresponding
> > StartupTimeline.h bit, so that there's no duplication.
> 
> By duplication, you mean the need for private and public fields?

I mean avoiding to have two places where the timestamps are defined.

> I would like to do that, but I need some way of taking timestamps from Java
> for events that happen before libxul is loaded.  I suppose post-libxul load,
> we can take timestamps directly in C, but I'm not sure that it's worth it.

APKOpen.cpp takes timestamps before libxul is loaded.
Comment 19 Nathan Froyd [:froydnj] 2013-06-28 05:31:09 PDT
(In reply to Mike Hommey [:glandium] from comment #18)
> (In reply to Nathan Froyd (:froydnj) from comment #17)
> > > I think it would be nicer to use a json file with a script that would
> > > generate StartupTimelineEvent.java completely, as well as the corresponding
> > > StartupTimeline.h bit, so that there's no duplication.
> > 
> > By duplication, you mean the need for private and public fields?
> 
> I mean avoiding to have two places where the timestamps are defined.

But there is only one place: StartupTimeline.h.  StartupTimelineEvent.java.in #include's that file for the exact reason of trying to maintain one source of truth.

> > I would like to do that, but I need some way of taking timestamps from Java
> > for events that happen before libxul is loaded.  I suppose post-libxul load,
> > we can take timestamps directly in C, but I'm not sure that it's worth it.
> 
> APKOpen.cpp takes timestamps before libxul is loaded.

Hm, I am going to have to take a closer look at the JNI setup, then.
Comment 20 Mike Hommey [:glandium] 2013-06-28 05:41:02 PDT
(In reply to Nathan Froyd (:froydnj) from comment #19)
> But there is only one place: StartupTimeline.h. 
> StartupTimelineEvent.java.in #include's that file for the exact reason of
> trying to maintain one source of truth.

My point is that StartupTimeline.h is not convenient for StartupTimelineEvent.java.in. So I'm suggesting to use another single place.
Comment 21 Nathan Froyd [:froydnj] 2013-06-28 08:44:05 PDT
My eideticker nytimes-load timeline values on a Galaxy Nexus:

91   browserAppOnCreateStart
489  browserAppOnCreateEnd
709  geckoThreadCreated
737  linkerInitialized
1159 librariesLoaded
1179 main
1592 startupCrashDetectionBegin
1639 AMI_startup_begin
1757 XPI_startup_begin
1866 XPI_bootstrap_addons_begin
1866 XPI_bootstrap_addons_end
1866 XPI_startup_end
1867 AMI_startup_end
1989 firstLoadChromeURI
2006 createTopLevelWindow
2427 androidBrowserStartupStart
2703 androidBrowserStartupEnd
2811 sessionRestored
2934 firstLoadURI

Ditching all the AMI stuff sure would be nice (200+ ms!).

The eideticker log says it takes us 4.5 seconds from the initial GET to the final GET.  I'm not sure how those numbers relate to the above (since the initial number is 8.8 seconds).
Comment 22 Nathan Froyd [:froydnj] 2013-06-28 09:28:44 PDT
Numbers with the addon manager disabled:

114  browserAppOnCreateStart
492  browserAppOnCreateEnd
754  geckoThreadCreated
791  linkerInitialized
1199 librariesLoaded
1218 main
1662 startupCrashDetectionBegin
1865 firstLoadChromeURI
1902 createTopLevelWindow
2321 androidBrowserStartupStart
2539 androidBrowserStartupEnd
2636 sessionRestored
2775 firstLoadURI
Comment 23 Brad Lassey [:blassey] (use needinfo?) 2013-06-28 15:37:54 PDT
Comment on attachment 768330 [details] [diff] [review]
add several startup timeline event timestamps to fennec

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

It might be easier to create events for these time stamps rather than creating a separate queue for them.

::: mobile/android/base/GeckoAppShell.java
@@ +345,4 @@
>              while (!gPendingEvents.isEmpty()) {
>                  GeckoEvent e = gPendingEvents.removeFirst();
>                  notifyGeckoOfEvent(e);
>              }

not that I think these will take a lot of time to send, but let's send the timestamps after clearing the queued up events.
Comment 24 Brad Lassey [:blassey] (use needinfo?) 2013-06-28 15:38:39 PDT
Comment on attachment 768340 [details] [diff] [review]
part 2 - add startupTimelineRecordStamp to GeckoAppShell

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

::: widget/android/AndroidJNI.cpp
@@ +835,5 @@
> +Java_org_mozilla_gecko_GeckoAppShell_startupTimelineRecordStamp(JNIEnv* jenv, jclass, jint event, jlong timestamp)
> +{
> +    // Make sure the preprocessing from StartupTimeline.h to Java works correctly.
> +    MOZ_ASSERT(0 <= event && event < StartupTimeline::MAX_EVENT_ID);
> +    mozilla::StartupTimelineRecordExternal(event, timestamp);

is this safe to call off the main thread?
Comment 25 Brad Lassey [:blassey] (use needinfo?) 2013-06-28 15:53:14 PDT
Comment on attachment 768326 [details] [diff] [review]
reflect StartupTimeline's enums into Java code

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

I like glandium's idea
Comment 26 Nathan Froyd [:froydnj] 2013-07-01 12:01:57 PDT
(In reply to Mike Hommey [:glandium] from comment #12)
> Comment on attachment 768326 [details] [diff] [review]
> reflect StartupTimeline's enums into Java code
> 
> Review of attachment 768326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Makefile.in
> @@ +1297,5 @@
> >  		cat $< | sed s,@APPNUM@,$$n,g >> $@ ; \
> >  	done
> >  
> > +StartupTimelineEvent.java: StartupTimelineEvent.java.in $(topsrcdir)/toolkit/components/startup/StartupTimeline.h
> > +	$(CPP) $(DEFINES) $(ACDEFINES) -I$(topsrcdir)/toolkit/components/startup $< | sed -e '/^#/d' > $@
> 
> I think it would be nicer to use a json file with a script that would
> generate StartupTimelineEvent.java completely, as well as the corresponding
> StartupTimeline.h bit, so that there's no duplication.

I tried doing this and ran into a problem: APKOpen.cpp in mozglue requires the definitions of the startup events.  But mozglue gets built before exports happen, so the logic in toolkit/components/startup/ to build the header for the events hasn't been run yet.

This worked before because mozglue reaches out through LOCAL_INCLUDES to grab the headers that it wants.  But in the brave new code generation world, it fails.

What's the right way forward here?  Go back to the $(CPP) approach, or reorder the build system phases, or some middle ground?
Comment 27 Nathan Froyd [:froydnj] 2013-07-01 12:02:19 PDT
See comment 26.
Comment 28 Mike Hommey [:glandium] 2013-07-01 19:52:15 PDT
I'm tempted to say StartupTimeline should move in mozglue, and that would make things easier (especially, we wouldn't have to pre-store the values from before libxul is loaded). But this is probably more work (and more risk) so I'd keep this for a followup that would remove the ugly hacks we'd put in place here.

One way to deal with this here is to define something like:

APKOpen.$(OBJ_SUFFIX): $(DEPTH)/toolkit/components/startup/StartupTimeline.h

$(DEPTH)/toolkit/components/startup/StartupTimeline.h:
        $(MAKE) -C $(@D) $(@F)

Replace StartupTimeline.h with whatever you call the generated header
Comment 29 Nathan Froyd [:froydnj] 2013-07-02 07:15:19 PDT
(In reply to Mike Hommey [:glandium] from comment #28)
> I'm tempted to say StartupTimeline should move in mozglue, and that would
> make things easier (especially, we wouldn't have to pre-store the values
> from before libxul is loaded). But this is probably more work (and more
> risk) so I'd keep this for a followup that would remove the ugly hacks we'd
> put in place here.

If that means TimeStamp can move to mfbt as a part of it, sounds like a good idea.

> One way to deal with this here is to define something like:
> 
> APKOpen.$(OBJ_SUFFIX): $(DEPTH)/toolkit/components/startup/StartupTimeline.h
> 
> $(DEPTH)/toolkit/components/startup/StartupTimeline.h:
>         $(MAKE) -C $(@D) $(@F)
> 
> Replace StartupTimeline.h with whatever you call the generated header

Oh man.  That's pretty gross.

I think it's slightly less gross to just generate a local-to-mozglue copy of the header using the script, and include that, rather than having to add $(DEPTH)/toolkit/components/startup/StartupTimeline.h to LOCAL_INCLUDES.  If you don't like that, we can do it your way instead.
Comment 30 Nathan Froyd [:froydnj] 2013-07-02 17:36:45 PDT
Created attachment 770544 [details] [diff] [review]
part 1a - change startup events to be generated from a json file

Here's the initial stab and generating the events from JSON.  glandium for the
build and mozglue parts, bsmedberg for the startup bits.
Comment 31 Nathan Froyd [:froydnj] 2013-07-02 17:37:14 PDT
Created attachment 770545 [details] [diff] [review]
part 1b - reflect startup events into java

This is a bit simpler now.
Comment 32 Mike Hommey [:glandium] 2013-07-02 18:12:41 PDT
Comment on attachment 770544 [details] [diff] [review]
part 1a - change startup events to be generated from a json file

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

Nits.

::: mozglue/android/Makefile.in
@@ +60,5 @@
> +
> +# toolkit/components/startup/ builds this same file, but it's not built and
> +# exported when mozglue is built.  So we build our own local copy here.
> +StartupTimelineEvents.h: $(events_json) $(events_generator)
> +	$(PYTHON) $(events_generator) $(events_json) c > $@

how about include $(topsrcdir)/toolkit/components/startup/gen-startup-events.mk, which would contain the above snippet? (including GARBAGE)

::: toolkit/components/startup/Makefile.in
@@ +24,5 @@
> +
> +StartupTimelineEvents.h: $(events_json) $(events_generator)
> +	$(PYTHON) $(events_generator) $(events_json) c > $@
> +
> +GARBAGE += StartupTimelineEvents.h

and include $(srcdir)/gen-startup-events.mk here should work.

::: toolkit/components/startup/StartupEvents.json
@@ +8,5 @@
> +  "SESSION_RESTORED" : "sessionRestored",
> +  "CREATE_TOP_LEVEL_WINDOW" : "createTopLevelWindow",
> +  "LINKER_INITIALIZED" : "linkerInitialized",
> +  "LIBRARIES_LOADED" : "librariesLoaded",
> +  "FIRST_LOAD_URI" : "firstLoadURI"

might be better to add a comma at the end here, so that adding new ones makes the patch easier to read. (not sure that works in json, though)

::: toolkit/components/startup/StartupTimeline.h
@@ +71,5 @@
>    static NS_EXTERNAL_VIS_(const char *) sStartupTimelineDesc[MAX_EVENT_ID];
>  };
>  
> +MOZ_STATIC_ASSERT(StartupTimeline::PROCESS_CREATION == static_cast<enum StartupTimeline::Event>(0),
> +		  "PROCESS_CREATION must be the first event!");

Is there really code that assumes that?

::: toolkit/components/startup/gen-startup-events.py
@@ +8,5 @@
> +
> +def outputCHeader(events):
> +    print "/* This file is auto-generated, see gen-startup-events.py.  */\n"
> +
> +    for (name, description) in events.iteritems():

you can drop the parenthesis here.

@@ +16,5 @@
> +    print "/* This file is auto-generated, see gen-startup-events.py.  */\n"
> +    print "package org.mozilla.gecko;\n"
> +    print "public class StartupTimelineEvent"
> +    print "{"
> +    for (i, name) in enumerate(events.iterkeys()):

and here

@@ +40,5 @@
> +    with open(eventFile, 'r') as f:
> +        events = json.load(f, object_pairs_hook=collections.OrderedDict)
> +        outputFunction(events)
> +
> +    sys.exit(0)

you can drop sys.exit here.
Comment 33 Nathan Froyd [:froydnj] 2013-07-03 08:31:43 PDT
Created attachment 770832 [details] [diff] [review]
part 1a - change startup events to be generated from a json file

Applied glandium's review comments.  Asking for re-review to check on the goodness of
gen-startup-events.mk
Comment 34 Brad Lassey [:blassey] (use needinfo?) 2013-07-03 11:19:45 PDT
Comment on attachment 770545 [details] [diff] [review]
part 1b - reflect startup events into java

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

::: mobile/android/base/Makefile.in
@@ +1188,3 @@
>  	@echo "JAR gecko-browser.jar"
>  	$(NSINSTALL) -D classes/gecko-browser
> +	$(JAVAC) $(JAVAC_FLAGS) -Xlint:all,-deprecation,-fallthrough -d classes/gecko-browser -classpath "jars/gecko-mozglue.jar:jars/gecko-util.jar:jars/sync-thirdparty.jar:jars/websockets.jar" $(addprefix $(srcdir)/,$(FENNEC_JAVA_FILES)) $(FENNEC_PP_JAVA_FILES) $(FENNEC_PP_JAVA_VIEW_FILES) $(addprefix $(srcdir)/,$(SYNC_JAVA_FILES)) $(SYNC_PP_JAVA_FILES) R.java StartupTimelineEvent.java

you should just be able to add StartupTimelineEvent.java to FENNEC_JAVA_FILES. Please do that instead.
Comment 35 Nathan Froyd [:froydnj] 2013-07-03 11:23:43 PDT
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #34)
> ::: mobile/android/base/Makefile.in
> @@ +1188,3 @@
> >  	@echo "JAR gecko-browser.jar"
> >  	$(NSINSTALL) -D classes/gecko-browser
> > +	$(JAVAC) $(JAVAC_FLAGS) -Xlint:all,-deprecation,-fallthrough -d classes/gecko-browser -classpath "jars/gecko-mozglue.jar:jars/gecko-util.jar:jars/sync-thirdparty.jar:jars/websockets.jar" $(addprefix $(srcdir)/,$(FENNEC_JAVA_FILES)) $(FENNEC_PP_JAVA_FILES) $(FENNEC_PP_JAVA_VIEW_FILES) $(addprefix $(srcdir)/,$(SYNC_JAVA_FILES)) $(SYNC_PP_JAVA_FILES) R.java StartupTimelineEvent.java
> 
> you should just be able to add StartupTimelineEvent.java to
> FENNEC_JAVA_FILES. Please do that instead.

I don't think that works because everything that looks at FENNEC_JAVA_FILES expects to find them in the srcdir, whereas we're generating StartupTimelineEvent.java in the objdir.
Comment 36 Benjamin Smedberg [:bsmedberg] 2013-07-03 11:29:07 PDT
Comment on attachment 770832 [details] [diff] [review]
part 1a - change startup events to be generated from a json file

I don't understand why it's valuable to have the startup events be in a JSON file. I also don't think that, as we're trying to remove all the rules from Makefiles and put them into moz.build, we should be introducing a fairly weird custom rule for this. If it really is valuable to auto-generate this, can we check in the resulting file so that the work is done by the people who change this file?
Comment 37 Nathan Froyd [:froydnj] 2013-07-03 11:42:18 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #36)
> I don't understand why it's valuable to have the startup events be in a JSON
> file. I also don't think that, as we're trying to remove all the rules from
> Makefiles and put them into moz.build, we should be introducing a fairly
> weird custom rule for this. If it really is valuable to auto-generate this,
> can we check in the resulting file so that the work is done by the people
> who change this file?

Comment 12 is where Mike originally asked for this.  I think the .h file can serve as a source of truth equally as well as a JSON file, but I don't have a preference other than that consensus is reached.

I don't like checking in the auto-generated file; I think doing that makes sense when the generation process requires semi-arcane tools, but not when the tools are included in-tree.  And if we do check in the generated file, we're going to need some build rules somewhere (ala http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#1348) to tell people how to regenerate the file.  But if the build system is smart enough to know when and provide directions on how to regenerate the file, why doesn't it just do that?

If moz.build is advanced enough that adding custom rules is doable, I can try to do that instead.  Mike?
Comment 38 Mike Hommey [:glandium] 2013-07-03 17:46:46 PDT
moz.build doesn't support custom rules at the moment.
Comment 39 Mike Hommey [:glandium] 2013-07-03 17:55:43 PDT
Comment on attachment 770832 [details] [diff] [review]
part 1a - change startup events to be generated from a json file

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

::: toolkit/components/startup/gen-startup-events.mk
@@ +1,1 @@
> +ifndef topsrcdir

you may want to add a safeguard against double-inclusion.

@@ +2,5 @@
> +$(error topsrcdir was not set)
> +endif
> +
> +ifndef events_file_kind
> +$(error events_file_kind was not set)

alternatively, you could make it default to 'h'.
Comment 40 Nathan Froyd [:froydnj] 2013-07-09 06:22:02 PDT
Pinging Benjamin for a response to comment 37.
Comment 41 Benjamin Smedberg [:bsmedberg] 2013-07-09 06:25:04 PDT
gps should approve the build change, if we are going to keep the JSON. I know gps had talked about a moz.build grammar for simple python build tools, but apparently that's just an idea still. We have a Q3 goal to move all build rules into moz.build, and this would make that harder for what seems to me relatively little gain. Given that, I strongly prefer avoiding the JSON step and just using a header file.
Comment 42 Nathan Froyd [:froydnj] 2013-07-09 06:30:28 PDT
Comment on attachment 770832 [details] [diff] [review]
part 1a - change startup events to be generated from a json file

Asking gps for feedback per comment 41, then.
Comment 43 Mike Hommey [:glandium] 2013-07-09 06:39:11 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #41)
> and this would make that harder for what seems to me relatively little gain.

The point is, either way there needs to be a build rule, because the header can't be used directly in java anyways.
Comment 44 Gregory Szorc [:gps] 2013-07-11 14:01:13 PDT
Comment on attachment 770832 [details] [diff] [review]
part 1a - change startup events to be generated from a json file

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

If I wanted to run the hardline, there are a number of things I think could be improved:

1) We should define the new functionality in moz.build files. We could either say "here is the JSON file." Or, we could even have a way of registering startup timeline events in moz.build files. Then, the build backend simply collects them from all the separate define sites then writes out the auto-generated header file from the aggregated set. The latter would remove the need of a separately #include'd .mk file.

2) I'd like new Python scripts to be installed in python/mozbuild/mozbuild/action directory and invoked using the new calling convention (see bug 891474).

3) The new Python file should be Python 3 compatible.

If you are feeling generous, I'd love to see things done this way. But, I'm not going to hold up checkin. Just don't get too attached to the code because we'll likely be changing things around in the next few months :)

::: toolkit/components/startup/gen-startup-events.mk
@@ +1,1 @@
> +ifndef topsrcdir

Need moar MPL.

::: toolkit/components/startup/moz.build
@@ +11,5 @@
>  MODULE = 'toolkitcomps'
>  
>  EXPORTS.mozilla += [
>      'StartupTimeline.h',
> +    'StartupTimelineEvents.h',

Since StartupTimelineEvents.h is auto-generated, it shouldn't be in EXPORTS. It will work today, but it will need changed for bug 890097.

Instead of declaring it here, could you just write the file into $(DIST)/include/StartupTimelineEvents.h from the newly-added rule in the make file?

If you leave this as-is, we'll catch it in bug 890097.

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