[1.x] Remove the Protocol Buffer usage #1419
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**Problem/Solution** We currently have three (3) implementations of Analysis persistence. Given that the "consistent" binary is the best, we should remove the protobuf one, so we don't need to update three code bases as we refactor for 2.x.
Friendseeker
force-pushed
the
consistent-fixes
branch
from
fd5088a to
b0e2c85
Compare
Friendseeker
commented
Oct 3, 2024
| @@ -142,6 +145,8 @@ class ConsistentAnalysisFormat(val mappers: ReadWriteMappers, sort: Boolean) { | |||
| out.int(ac.apiHash()) | |||
| out.bool(ac.hasMacro) | |||
| out.string(ac.provenance()) | |||
| out.int(ac.extraHash()) | |||
| out.int(ac.bytecodeHash()) | |||
There was a problem hiding this comment.
Need to forward port
Friendseeker
commented
Oct 3, 2024
| @@ -166,6 +171,8 @@ class ConsistentAnalysisFormat(val mappers: ReadWriteMappers, sort: Boolean) { | |||
| val ah = in.int() | |||
| val hm = in.bool() | |||
| val p = in.string() | |||
| val eh = in.int() | |||
| val bh = in.int() | |||
There was a problem hiding this comment.
Need to forward port
Friendseeker
commented
Oct 3, 2024
| @@ -178,7 +185,7 @@ class ConsistentAnalysisFormat(val mappers: ReadWriteMappers, sort: Boolean) { | |||
| val comp = | |||
| if (storeApis) Companions.of(readClassLike(in), readClassLike(in)) | |||
| else APIs.emptyCompanions | |||
| AnalyzedClass.of(ts, name, SafeLazyProxy.strict(comp), ah, nameHashes, hm, ah, p) | |||
| AnalyzedClass.of(ts, name, SafeLazyProxy.strict(comp), ah, nameHashes, hm, eh, p, bh) | |||
There was a problem hiding this comment.
Need to forward port
Friendseeker
force-pushed
the
consistent-fixes
branch
from
b0e2c85 to
b8b7312
Compare
Friendseeker
commented
Oct 3, 2024
| "name": "bytecodeHash", | ||
| "type": "int", | ||
| "default": "0", | ||
| "since": "1.11.0", |
There was a problem hiding this comment.
Can change it to some other version if we do not plan to release 1.11.0 next.
Iterable won't suffice. With iterable the hash can be order specific.
Friendseeker
force-pushed
the
consistent-fixes
branch
2 times, most recently
from
502f7ce to
b7bf35c
Compare
|
Would appreciate any tip on why Ubuntu Java 8 CI might be failing... I tried running |
Friendseeker
force-pushed
the
consistent-fixes
branch
from
178d6e4 to
b7bf35c
Compare
|
I am so, so confused... The (8, 1) job passed on my own repo... Why is it failing there? https://github.com/Friendseeker/zinc/actions/runs/11173227513/job/31060972851 |
When upstream project macro is recompiled, we need to hash bytecodes the macro calls into for child project to detect upstream macro API change.
Friendseeker
force-pushed
the
consistent-fixes
branch
from
b0e9122 to
88150aa
Compare
Friendseeker
commented
Oct 4, 2024
| @@ -344,6 +361,8 @@ class ConsistentAnalysisFormat(val mappers: ReadWriteMappers, sort: Boolean) { | |||
| wrS("inheritance.external", rs.inheritance.external) | |||
| wrS("localInheritance.internal", rs.localInheritance.internal) | |||
| wrS("localInheritance.external", rs.localInheritance.external) | |||
| wrS("macroExpansion.internal", rs.macroExpansion.internal) | |||
| wrS("macroExpansion.external", rs.macroExpansion.external) | |||
There was a problem hiding this comment.
Need to forward port
Friendseeker
force-pushed
the
consistent-fixes
branch
from
91d02bd to
e1593f3
Compare
Friendseeker
force-pushed
the
consistent-fixes
branch
from
e1593f3 to
d417c73
Compare
@adpi2 @eed3si9n Would appreciate advice from both of you. This PR is otherwise good to go but I am struggling to even form an hypothesis on what is causing the timeouts. In fact I cannot even reproduce the same timeout failure on my M1 mac running Java 8. Update: Still no clue on what is going on, but I suspect the problem is in Parallel GZIP compression. Here's a thread dump from a failed CI run. There's a queue getting blocked and threads doing IO. |
|
Yes it is the GZIP Parallel compression causing the transient CI failures... I did Github Action runs on two zinc branches on my fork, one using I guess I shall remove the use of |
|
Here's the benchmark result. Shall switch to use Snappy as it is slightly faster than ParallelGZIP. LZ4 would likely be even faster but LZ4-java is dead.
Result compiled from https://github.com/Friendseeker/zinc/actions/runs/11189486493/job/31109922634 |
Friendseeker
force-pushed
the
consistent-fixes
branch
from
5bc5c55 to
cf024b2
Compare
|
@Friendseeker Thanks for the contribution! Here's my opinion: I think demonstrating that we have the capability to no longer depend on timestamp is great for both Zinc 1.x and 2.x, and I appreciate the effort you've put in here. However, I don't think it's a good idea to actually go through with the removal in Zinc / sbt 1.x series, since we'd end up taking on known-unknown risks, the risk budget I'd rather focus on forwarding sbt / Zinc 2.x. |
I shall open another PR that targets 2.x porting all the changes in this PR, along with making a PR targeting 1.x only porting the bug-fixes. Indeed I was also thinking that such major change should only target 2.x branch. In fact one of the main reason I targeted 1.x branch in the first place is that all scripted tests can be ran to iron out problems. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR back ports #1388 along with implementing #1412, fixing gaps in consistent analysis format and replacing use of
ParallelGzipOutputStreamwith Snappy.Implementing #1412 (Content Hashing)
This PR replaces timestamp in
detectAPIChangeswith two types of content hashes.bytecodeHashcomputes content hash of all class files associated with a source file.extraBytecodeHashis a hash of all upstream classes'sbytecodeHash, similar to #1388 (comment).For a non-macro class, we employ
bytecodeHashas a direct replacement of compilation timestamp.For a macro class, there are cases for which the macro has same
bytecodeHashafter the macro is recompiled but the macro's upstream dependency is changed hence the macro expansion result would be different. Therefore if a class is macro we also compare byextraBytecodeHashto detect such change.Fixing gaps in consistent analysis format
The following two issues are fixed
DependencyByMacroExpansion.extraHash.Replacing use of
ParallelGzipOutputStreamwith SnappyThere were sporadic CI scripted test timeout failures such as https://github.com/sbt/zinc/actions/runs/11173856057/job/31062605736 and https://github.com/sbt/zinc/actions/runs/11173276915/job/31061098761. These failures commonly happen with (ubuntu-latest, 8, 1) but sometimes happen with (windows-latest, 8, 2). It only happens with Java 8. These failures also cease to happen when we run scripted test one by one.
The CI failures disappear when we use any other compression stream than
ParallelGzipOutputStream. Hence this PR uses Snappy to compress/decompress Analysis instead. Snappy is slightly faster thanParallelGzipOutputStreamand is an actively maintained library. c.c. #1419 (comment) for raw benchmark result.