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)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: froydnj, Unassigned)
References
Details
Attachments
(6 files, 3 obsolete files)
(deleted),
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
gps
:
feedback+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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•11 years ago
|
||
Seems helpful to know how long our javascript initialization is taking.
Attachment #768327 -
Flags: review?(blassey.bugs)
Reporter | ||
Comment 3•11 years ago
|
||
We need to be able to communicate timestamps to Gecko through JNI.
Attachment #768328 -
Flags: review?(blassey.bugs)
Reporter | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
||
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)
Comment 6•11 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 7•11 years ago
|
||
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•11 years ago
|
||
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 9•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #768327 -
Flags: review?(mark.finkle) → review+
Reporter | ||
Comment 10•11 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•11 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 12•11 years ago
|
||
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 13•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 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.
Comment 18•11 years ago
|
||
(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•11 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.
Comment 20•11 years ago
|
||
(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•11 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•11 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
Updated•11 years ago
|
Attachment #768340 -
Flags: review?(blassey.bugs) → review+
Comment 23•11 years ago
|
||
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 24•11 years ago
|
||
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•11 years ago
|
||
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•11 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?
Comment 28•11 years ago
|
||
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•11 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•11 years ago
|
||
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•11 years ago
|
||
This is a bit simpler now.
Attachment #770545 -
Flags: review?(blassey.bugs)
Comment 32•11 years ago
|
||
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•11 years ago
|
||
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 34•11 years ago
|
||
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•11 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 36•11 years ago
|
||
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•11 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)
Comment 38•11 years ago
|
||
moz.build doesn't support custom rules at the moment.
Flags: needinfo?(mh+mozilla)
Comment 39•11 years ago
|
||
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•11 years ago
|
||
Pinging Benjamin for a response to comment 37.
Flags: needinfo?(benjamin)
Comment 41•11 years ago
|
||
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•11 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)
Comment 43•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #770832 -
Flags: review?(benjamin)
Comment 44•11 years ago
|
||
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+
Comment 45•4 years ago
|
||
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
Assignee | ||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•