Closure compiler flags for advanced optimizations and J2CL_OPTIMIZED_DEFS

[niloc132]

Documentation around suggested defines seems incomplete/inaccurate. From https://github.com/google/j2cl/blob/master/docs/best-practices.md#closure-compiler-flags:

For the best results, use J2CL_OPTIMIZED_DEFS which enables all the advanced optimizations:

 load("//build_defs:rules.bzl", "J2CL_OPTIMIZED_DEFS")

 js_binary(
     name = "optimized_j2cl_app",
     defs = J2CL_OPTIMIZED_DEFS,
     deps = [":js_lib"],
 )

However, J2CL_OPTIMIZED_DEFS only contains goog.DEBUG=false: https://github.com/google/j2cl/blob/c6c78be/build_defs/internal_do_not_use/j2cl_js_common.bzl#L85

 J2CL_OPTIMIZED_DEFS = [
     "--define=goog.DEBUG=false",
 ]

Is it accurate to say that this enables all advanced optimizations? Should either that array be updated to include others, or the docs updated to include a more comprehensive list of "defines that enable all advanced optimizations"?

From my list, we also want to add at least these three flags to either the docs or to J2CL_OPTIMIZED_DEFS:

• --define=jre.checks.checkLevel=MINIMAL (mentioned on the linked page)
• --define=jre.checkedMode=DISABLED (not mentioned on the linked page, but used by gwt's InternalPreconditions and also by https://github.com/google/j2cl/blob/c6c78be/transpiler/javatests/com/google/j2cl/transpiler/optimization/j2cl_optimization_test.bzl#L20)
• --define=jsinterop.checks=DISABLED (assuming jsinterop-base is being used)

[gkdn]

However, J2CL_OPTIMIZED_DEFS only contains goog.DEBUG=false:

I think the main issue is; the document is written by having internal users in mind. We have a larger list internally but that's mostly covered by ADVANCED optimization which is enabled by default in closure_js_binary. I need to cross check reference app size to see if we are missing any flags.

--define=jre.checks.checkLevel=MINIMAL (mentioned on the linked page)

As noted in the documentation: "Note that this configuration should be set to same value for both debugging and production in order to detect user code that incorrectly relies on specific exceptions to be thrown."

So this should not to be only set for optimizations hence putting the flag in J2CL_OPTIMIZED_DEFS is not a good idea..

We historically considered this as an opt-in semantic change for both GWT and J2CL. However given that all our users are opting into this, maybe we should just change the default value.

--define=jre.checkedMode=DISABLED

This is a big no; this is not an optimization. This was not intended to be touched by users but currently might be needed to work around cross-frame issues for development mode only - which we should have a better solution for.

--define=jsinterop.checks=DISABLED

This is already the default.

[niloc132]

Okay, I think I must have misunderstood the email which initially suggested some of these to me - I'll remove these as defaults, and make sure to prominently link to the docs to let devs make up their own minds for these.

  --define=jre.checkedMode=DISABLED

This is a big no; this is not an optimization. This was not intended to be touched by users but currently might be needed to work around cross-frame issues for development mode only - which we should have a better solution for.

Just to confirm, we do expect that this is always driven by goog.DEBUG then? https://github.com/google/j2cl/blob/fb66a0d/jre/java/java/lang/jre.js#L25-L27

/** @define {string} */
jre.checkedMode =
    goog.define('jre.checkedMode', goog.DEBUG ? 'ENABLED' : 'DISABLED')

This does seem to follow, since superdevmode sets jre.debugMode sets jre.checkedMode in gwt2.

[gkdn]

Yes we expect it to be driven by goog.DEBUG.

[niloc132]

Thanks for your help on this, one last question (filed here as it is related to this issue, and also because it is in the j2cl-only code in jsinterop-base): in https://github.com/google/jsinterop-base/blob/18973cb/java/jsinterop/base/jsinterop.js#L25-L28 there is a comment which says

// Note that disabling checking only disables it for production.

but between that and https://github.com/google/jsinterop-base/blob/63c2d8d/java/jsinterop/base/InternalPreconditions.java#L31 it doesn't appear that this is only disabled for production, at least not in open source? Should this also be set to enable it when goog.DEBUG is true, as with jre.checkedMode?

[gkdn]

IS_ASSERTED is backed by jre.checkedMode which is driven from goog.DEBUG.

[niloc132]

Got it, thank you - it is meant to be read "when disabled, these errors will still happen (but as AssertionExceptions instead of ClassCastException), except in production, when they be disabled entirely". I was instead parsing it as something closer to "this type check will be enabled in development, but not in production (like jre.checkedMode)".