Closed Bug 1448428 Opened 7 years ago Closed 5 years ago

Split generated JNI wrappers into multiple .h and .cpp files

Categories

(Firefox Build System :: General, enhancement, P1)

3 Branch
enhancement

Tracking

(firefox78 fixed)

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: nalexander, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

(Whiteboard: [geckoview:p3][geckoview:m78])

Attachments

(6 files, 1 obsolete file)

(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
I have patches in progress to generate the JNI wrappers from the (compiled) Java code rather than checking generated artifacts into the tree. While testing, I notice that any change to the wrappers triggers a fairly significant |mach build binaries| cycle, since the places that include "GeneratedJNIWrappers.h" have grown significantly since inception. One way that we might improve this is to make the wrapper generation produce multiple .h and .cpp files, name them in some standard way, and then achieve an "include what you use" win when editing the wrappers. Since we have ~300 WrapForJNI uses in the tree right now, it's worth pursuing this sooner rather than later, I think.
jchen: could you suggest a naming scheme? Could we use JNI's naming scheme, which turns package.foo.Class.function into package_foo_Class_function? Maybe we want package_foo_Class.{h,cpp}? Or we could use the exact same naming scheme as we do for the generated SDK bindings? In some way, let's unify the presentation of Java classes to C++ code, and allow for finer grained build dependencies.
Flags: needinfo?(nchen)
(In reply to Nick Alexander :nalexander from comment #0) > I have patches in progress to generate the JNI wrappers from the (compiled) > Java code rather than checking generated artifacts into the tree. I don't these are required for this ticket to generate discussion and make progress, but the patches will be submitted for review not so long from now and are on try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1266d46d2fa8c19b99ea6a02ee25f1d861837717
(In reply to Nick Alexander :nalexander from comment #1) > jchen: could you suggest a naming scheme? Could we use JNI's naming scheme, > which turns package.foo.Class.function into package_foo_Class_function? > Maybe we want package_foo_Class.{h,cpp}? > > Or we could use the exact same naming scheme as we do for the generated SDK > bindings? For SDK bindings, we discard the package name and just use the class name (i.e. Class.{h,cpp}). We should do something similar here, preferably with the include path under mozilla/java (i.e. #include "mozilla/java/Class.h"), because the generated bindings are under the mozilla::java namespace.
Flags: needinfo?(nchen)
Version: Version 3 → 3 Branch

DO NOT LAND - This handles the build system bits. What is still
needed is to update the consumers of the .h files (all the #include
lines) to use the fine-grained imports.

I did some of the build bits for this ticket, but I don't have a good way to determine what new includes are needed and I don't want to spend the time to work them out by hand. etoop, can you find an assignee for that? The pay off is that adding @WrapForJNI should recompile fewer files, which I expect will produce faster builds as GV devs evolve the JNI wrapper signatures.

Flags: needinfo?(etoop)

adding geckoview whiteboard tag to send this to triage.

Flags: needinfo?(etoop)
Whiteboard: [geckoview]

P3 nice to have for internal GV developer ergonomics

Priority: -- → P3
Whiteboard: [geckoview] → [geckoview:p3]

This handles the build system bits.

The subsequent patch will restore the "unified"
GeneratedJNI{Natives,Wrappers}.h header files, and will be folded into
this one before landing.

What will still remain is to update the consumers of the .h files (all
the current #include lines) to use the fine-grained imports. At that
time the "unified" header files can be removed entirely.

Assignee: nobody → nalexander
Status: NEW → ASSIGNED

It's not trivial to split the existing "unified" include declaration
into granular include declarations, so we continue generating a
unified header that can be incrementally abandoned until it can be
jettisoned.

Depends on D58572

This establishes a "high-water" mark that will make new files not be
"unified", and will allow to burn down the list of files require
unification.

Depends on D58573

Attachment #9073445 - Attachment is obsolete: true

I needed something to ease back into actual work, so I took a second look at this. Try build is percolating at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1da8a6bb0cf2eb4d748aad9bce83b463c6a3221d.

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:nalexander, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(nalexander)

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #13)

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:nalexander, could you have a look please?
For more information, please visit auto_nag documentation.

Sure! I'm passing this work over to :aklotz (who I have assigned). This is ready to go, modulo that :snorp asked for a more reasonable file name scheme. I think that will be on Aaron (or on follow-up).

Assignee: nalexander → aklotz
Flags: needinfo?(nalexander)

We update the name generation code to dump the files into:

OBJDIR/widget/android/jni/GeneratedJNI{Natives, Wrappers}

which are then exported to mozilla/jni/natives and mozilla/jni/wrappers

Depends on D58574

I added part 1d to fix the naming scheme. I'll file follow-up bugs for transitioning remaining includes, as well as perhaps autogenerating dependencies (which I have discussed the possibility of with Nathan, and we can experiment with).

Priority: P3 → P1
Pushed by aklotz@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3a3c734343f3 Part 1a: Split generated JNI wrappers into multiple files. r=snorp,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/92ca5adb7eb3 Part 1b: Maintain "unified" GeneratedJNI{Natives,Wrappers}.h header. r=snorp,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/c48617a975e9 Part 1c: Allow incremental transition away from "unified" GeneratedJNI{Natives,Wrappers}.h header. r=snorp,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/da732f0c37d5 Part 1d: Change naming scheme for generated source files; r=snorp,nalexander,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/192294c1413a Part 2: Transition "HardwareCodecCapabilityUtils" away from "unified" GeneratedJNI{Natives,Wrappers}.h header. r=snorp,geckoview-reviewers

grr, probably some dirs that need updating since this patchset was first written...

Flags: needinfo?(aklotz)
Pushed by aklotz@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/298f90202eda Part 1a: Split generated JNI wrappers into multiple files. r=snorp,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/9464ad22d412 Part 1b: Maintain "unified" GeneratedJNI{Natives,Wrappers}.h header. r=snorp,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/bf98f3293602 Part 1c: Allow incremental transition away from "unified" GeneratedJNI{Natives,Wrappers}.h header. r=snorp,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/6f2d5ac0672d Part 1d: Change naming scheme for generated source files; r=snorp,nalexander,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/ba407e8fac92 Part 2: Transition "HardwareCodecCapabilityUtils" away from "unified" GeneratedJNI{Natives,Wrappers}.h header. r=snorp,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/6a8277223f6d Part 3: Transition EnterpriseRoots away from unified GeneratedJNIWrappers.h header; r=keeler
Whiteboard: [geckoview:p3] → [geckoview:p3][geckoview:m78]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: