Closed Bug 887824 Opened 11 years ago Closed 4 years ago

add more startup timeline events to fennec

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: froydnj, Unassigned)

References

Details

Attachments

(6 files, 3 obsolete files)

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.
Attached patch reflect StartupTimeline's enums into Java code (obsolete) (deleted) — Splinter Review
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)
Seems helpful to know how long our javascript initialization is taking.
Attachment #768327 - Flags: review?(blassey.bugs)
Attached patch add startupTimelineRecordStamp to GeckoAppShell (obsolete) (deleted) — Splinter Review
We need to be able to communicate timestamps to Gecko through JNI.
Attachment #768328 - Flags: review?(blassey.bugs)
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)
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)
Blocks: 807322
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?
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+
(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.
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.
(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.
(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.
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).
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-
(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?
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)
(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.
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)
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+
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+
(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?
(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+
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)
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+
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: