add more startup timeline events to fennec

NEW
Unassigned

Status

()

Firefox for Android
General
4 years ago
4 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
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.
Attachment #768326 - Flags: review?(mh+mozilla)
Attachment #768326 - Flags: review?(blassey.bugs)
(Reporter)

Comment 2

4 years ago
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.
Attachment #768327 - Flags: review?(blassey.bugs)
(Reporter)

Comment 3

4 years ago
Created attachment 768328 [details] [diff] [review]
add startupTimelineRecordStamp to GeckoAppShell

We need to be able to communicate timestamps to Gecko through JNI.
Attachment #768328 - Flags: review?(blassey.bugs)
(Reporter)

Comment 4

4 years ago
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.
Attachment #768330 - Flags: review?(blassey.bugs)
(Reporter)

Comment 5

4 years ago
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.
Attachment #768331 - Flags: review?(bugs)
(Reporter)

Updated

4 years ago
Blocks: 807322

Comment 6

4 years ago
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.
Attachment #768331 - Flags: review?(bugs) → review+
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?
(Reporter)

Comment 8

4 years ago
Created attachment 768340 [details] [diff] [review]
part 2 - add startupTimelineRecordStamp to GeckoAppShell

Here, let's try attaching something that compiles after furious rebasing.
Attachment #768328 - Attachment is obsolete: true
Attachment #768328 - Flags: review?(blassey.bugs)
Attachment #768340 - Flags: review?(blassey.bugs)
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
Attachment #768327 - Flags: review?(blassey.bugs) → review?(mark.finkle)
Attachment #768327 - Flags: review?(mark.finkle) → review+
(Reporter)

Comment 10

4 years ago
(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.
(Reporter)

Comment 11

4 years ago
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 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.
Attachment #768326 - Flags: review?(mh+mozilla) → review-
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.
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
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)
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.
(Reporter)

Comment 17

4 years ago
(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.
(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.
(Reporter)

Comment 19

4 years ago
(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.
(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.
(Reporter)

Comment 21

4 years ago
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).
(Reporter)

Comment 22

4 years ago
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
Attachment #768340 - Flags: review?(blassey.bugs) → review+
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.
Attachment #768330 - Flags: review?(blassey.bugs) → review+
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 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
Attachment #768326 - Flags: review?(blassey.bugs) → review-
(Reporter)

Comment 26

4 years ago
(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?
(Reporter)

Comment 27

4 years ago
See comment 26.
Flags: needinfo?(mh+mozilla)
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
Flags: needinfo?(mh+mozilla)
(Reporter)

Comment 29

4 years ago
(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.
(Reporter)

Comment 30

4 years ago
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.
Attachment #768326 - Attachment is obsolete: true
Attachment #770544 - Flags: review?(mh+mozilla)
Attachment #770544 - Flags: review?(benjamin)
(Reporter)

Comment 31

4 years ago
Created attachment 770545 [details] [diff] [review]
part 1b - reflect startup events into java

This is a bit simpler now.
Attachment #770545 - Flags: review?(blassey.bugs)
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.
Attachment #770544 - Flags: review?(mh+mozilla) → review+
(Reporter)

Comment 33

4 years ago
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
Attachment #770544 - Attachment is obsolete: true
Attachment #770544 - Flags: review?(benjamin)
Attachment #770832 - Flags: review?(mh+mozilla)
Attachment #770832 - Flags: review?(benjamin)
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.
Attachment #770545 - Flags: review?(blassey.bugs) → review+
(Reporter)

Comment 35

4 years ago
(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 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?
(Reporter)

Comment 37

4 years ago
(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?
Flags: needinfo?(mh+mozilla)
moz.build doesn't support custom rules at the moment.
Flags: needinfo?(mh+mozilla)
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'.
Attachment #770832 - Flags: review?(mh+mozilla) → review+
(Reporter)

Comment 40

4 years ago
Pinging Benjamin for a response to comment 37.
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
(Reporter)

Comment 42

4 years ago
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.
Attachment #770832 - Flags: feedback?(gps)
(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.
Attachment #770832 - Flags: review?(benjamin)
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.
Attachment #770832 - Flags: feedback?(gps) → feedback+
You need to log in before you can comment on or make changes to this bug.