Enable embedding sources inside the sourcemaps json as sourcesContent #9936
Conversation
vegegoku
force-pushed
the
source-content-2-11-test
branch
2 times, most recently
from
a80d0a6
to
ddf1364
Compare
There was a problem hiding this comment.
Some updates from private chat working on this feature:
- We've learned that SDM won't be improved by this, as the various linking activity doesn't copy source files for in SDM, only in prod code with sourcemaps enabled. The "sourcesContent" part of this patch will only improve SDM in cases where the debugger loads many many sources, to the point where that network traffic could be time better spent reading a giant sourcemap file.
- This is still a legit way to slightly improve prod build times for projects that deploy with sourcemaps (not many, but they exist), though it might be counterproductive for those only using sourcemaps for stack trace deobfuscation
- For these reasons, we're leaning towards a config property in CompilerProperties.gwt.xml that would activate this new feature.
- The patch still has value simply by updating the sourcemap library: both to potentially fix any issues in the last 8ish years since this was updated, and to drop the server-side runtime dependency on protobuf once and for all.
There is also other cleanup possible for compiler.useSourceMaps (since it should work in all browsers today, and not need extra wiring in CoreWithUserAgent), but let's leave that to another patch.
vegegoku
force-pushed
the
source-content-2-11-test
branch
4 times, most recently
from
11a0f27
to
b39f264
Compare
vegegoku
force-pushed
the
source-content-2-11-test
branch
from
b39f264
to
7584780
Compare
vegegoku
force-pushed
the
source-content-2-11-test
branch
from
7584780
to
8f42b06
Compare
vegegoku
force-pushed
the
source-content-2-11-test
branch
from
8f42b06
to
66f39c9
Compare
There was a problem hiding this comment.
Looks like there may be more classpaths that should be expanded, or code that needs to be corrected to stop walking classes it should ignore (annotations mostly?)
Example from the build:
requestfactory-server:
[java] Could not find class file for com/google/auto/value/AutoValue
[java] Could not find class file for com/google/auto/value/AutoValue$Builder
[java] May 12, 2024 4:55:17 AM com.google.auto.value.AutoValue
[java] SEVERE: Could not find class file for com/google/auto/value/AutoValue
[java] May 12, 2024 4:55:17 AM com.google.auto.value.AutoValue.Builder
[java] SEVERE: Could not find class file for com/google/auto/value/AutoValue$Builder
[java] Could not find class file for com/google/errorprone/annotations/CanIgnoreReturnValue
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.CanIgnoreReturnValue
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/CanIgnoreReturnValue
[java] Could not find class file for com/google/errorprone/annotations/DoNotCall
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.DoNotCall
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/DoNotCall
[java] Could not find class file for com/google/errorprone/annotations/InlineMe
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.InlineMe
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/InlineMe
[java] Could not find class file for com/google/errorprone/annotations/DoNotMock
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.DoNotMock
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/DoNotMock
[java] Could not find class file for com/google/errorprone/annotations/InlineMeValidationDisabled
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.InlineMeValidationDisabled
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/InlineMeValidationDisabled
[java] Could not find class file for com/google/errorprone/annotations/CompatibleWith
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.CompatibleWith
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/CompatibleWith
[java] Could not find class file for com/google/errorprone/annotations/Immutable
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.Immutable
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/Immutable
[java] Could not find class file for com/google/errorprone/annotations/ForOverride
[java] Could not find class file for com/google/errorprone/annotations/CheckReturnValue
[java] Could not find class file for com/google/errorprone/annotations/RestrictedApi
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.ForOverride
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/ForOverride
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.CheckReturnValue
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/CheckReturnValue
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.RestrictedApi
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/RestrictedApi
[java] Could not find class file for com/google/errorprone/annotations/concurrent/GuardedBy
[java] May 12, 2024 4:55:17 AM com.google.errorprone.annotations.concurrent.GuardedBy
[java] SEVERE: Could not find class file for com/google/errorprone/annotations/concurrent/GuardedBy
| if (nonNull(cis)) { | ||
| try { | ||
| cis.close(); | ||
| } catch (IOException e) { | ||
| logger.log(TreeLogger.Type.WARN, "Error closing input stream ", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could also be
| if (nonNull(cis)) { | |
| try { | |
| cis.close(); | |
| } catch (IOException e) { | |
| logger.log(TreeLogger.Type.WARN, "Error closing input stream ", e); | |
| } | |
| } | |
| Utility.close(cis); |
| import com.google.gwt.thirdparty.guava.common.collect.Maps; | ||
| import java.io.ByteArrayOutputStream; |
There was a problem hiding this comment.
from checkstyle:
/home/runner/work/gwt/gwt/gwt/dev/core/src/com/google/gwt/core/linker/SymbolMapsLinker.java:49:1: warning: 'java.io.ByteArrayOutputStream' should be separated from previous imports. (com.puppycrawl.tools.checkstyle.checks.imports.ImportOrderCheck)
| import com.google.gwt.thirdparty.guava.common.collect.Maps; | |
| import java.io.ByteArrayOutputStream; | |
| import com.google.gwt.thirdparty.guava.common.collect.Maps; | |
| import java.io.ByteArrayOutputStream; |
| totalPrefixLines, | ||
| sourceMapString, partialPath); |
There was a problem hiding this comment.
| totalPrefixLines, | |
| sourceMapString, partialPath); | |
| totalPrefixLines, sourceMapString, partialPath); |
| new SourceMapGeneratorV3.ExtensionMergeAction() { | ||
| @Override | ||
| public Object merge(String extKey, Object oldVal, Object newVal) { | ||
| return newVal; | ||
| } | ||
| }); |
There was a problem hiding this comment.
IJ suggests replacing this with a lambda, seems like a good improvement?
| new SourceMapGeneratorV3.ExtensionMergeAction() { | |
| @Override | |
| public Object merge(String extKey, Object oldVal, Object newVal) { | |
| return newVal; | |
| } | |
| }); | |
| (extKey, oldVal, newVal) -> newVal); |
| Iterator<Entry<String, Object>> extensions = section.getExtensions().entrySet().iterator(); | ||
|
|
||
| while (extensions.hasNext()) { |
There was a problem hiding this comment.
| Iterator<Entry<String, Object>> extensions = section.getExtensions().entrySet().iterator(); | |
| while (extensions.hasNext()) { | |
| for (Entry<String, Object> entry : section.getExtensions().entrySet()) { |
| @@ -131,6 +131,7 @@ public int compare(SelectionProperty o1, SelectionProperty o2) { | |||
| private ResourceOracle publicResourceOracle; | |||
|
|
|||
| private final SortedSet<SelectionProperty> selectionProperties; | |||
| private ModuleDef module; | |||
| @@ -537,6 +539,10 @@ public void produceOutput(TreeLogger logger, ArtifactSet artifacts, | |||
| } | |||
| } | |||
|
|
|||
| public ModuleDef getModule() { | |||
There was a problem hiding this comment.
As we discussed when first working out how to plumb this, I don't really like exposing this here. I don't have a good enough answer to ask you to change it, so just documenting my thoughts in case something strikes one of us (or someone sees this PR in a few years and wants to know why on earth we made this change).
One option is to make a specific getter, shouldEmbedSourcesInModuleSourcemaps() sort of thing that just does return module.isEmbedSourcesContent(). That would better match getModuleName()/getModuleFunctionName()/getModuleLastModified() - though those three are part of LinkerContext itself. I don't think this should be added to LinkerContext.
I dislike this option the least. However, it does suggest that we should do the same in CompilerContext - but CompilerContext has a bunch of other calls to getModule() already, so it feels a bit inconsistent.
Another option would be to remove the other three getters, and have all the callers instead call getModule().getName() and so on. This provides access to too much of ModuleDef I think, and it isn't immutable, so I really don't care for this.
Separately, I don't love the name isEmbedSourcesContent as a method on ModuleDef, nor do I love that the method exists there at all. No other properties exist on ModuleDef itself, so it seems silly to have this one be special enough to be there. That said, it is needed by both the various pre/compile stages and linkers too, so it either needs to be somewhere very accessible, or needs to be duplicated - so I think I'm okay with it staying in ModuleDef or pushing it down into Properties (so you have to call getModule().getProperties.isBlahBlah() - that does happen in other places, so it wouldn't be terrible). Renaming it: how about we get SourceMaps in the name somehow - shouldEmbedSourceMapContents() perhaps? sourceMapContentsEmbedEnabled()?
This PR updates the rebased version of sourcemaps jar and other dependencies to support embedding the source files content into the source maps sourcesContent field.
requires : gwtproject/tools#31
Fixes #9752 #9778 #9790