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

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

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.

Comment 2

[Nick Alexander :nalexander [he/him]]

(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

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

(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.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1448428 - DO NOT LAND - Split generated JNI wrappers into multiple files.

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.

Comment 5

[Nick Alexander :nalexander [he/him]]

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.

Comment 6

[Emily Toop (:fluffyemily)]

adding geckoview whiteboard tag to send this to triage.

Comment 7

[Chris Peterson [:cpeterson]]

P3 nice to have for internal GV developer ergonomics

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1448428 - Part 1a: Split generated JNI wrappers into multiple files. r?snorp

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.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1448428 - Part 1b: Maintain "unified" GeneratedJNI{Natives,Wrappers}.h header. r?snorp

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

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1448428 - Part 1c: Allow incremental transition away from "unified" GeneratedJNI{Natives,Wrappers}.h header. r?snorp

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

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1448428 - Part 2: Transition "HardwareCodecCapabilityUtils" away from "unified" GeneratedJNI{Natives,Wrappers}.h header. r?snorp

Depends on D58574

Comment 12

[Nick Alexander :nalexander [he/him]]

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.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 14

[Nick Alexander :nalexander [he/him]]

(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).

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1448428 - Part 1d: Change naming scheme for generated source files; r=snorp!,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

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

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).

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Razvan Maries]

Backed out for build bustages at SocketProcessParent.cpp.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=HJqz0_u1Rg-3OA46-bgpkQ-0&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&searchStr=b&revision=192294c1413a95391a330f1390967894c547d914

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302007026&repo=autoland&lineNumber=18701

Backout: https://hg.mozilla.org/integration/autoland/rev/5e05ebce1da8b57a31df92ce710186413335642b

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1448428 - Part 3: Transition EnterpriseRoots away from unified GeneratedJNIWrappers.h header; r=keeler

Depends on D58575

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/298f90202eda
https://hg.mozilla.org/mozilla-central/rev/9464ad22d412
https://hg.mozilla.org/mozilla-central/rev/bf98f3293602
https://hg.mozilla.org/mozilla-central/rev/6f2d5ac0672d
https://hg.mozilla.org/mozilla-central/rev/ba407e8fac92
https://hg.mozilla.org/mozilla-central/rev/6a8277223f6d