Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance of parsing unknown fields in Java #9371

Merged
merged 1 commit into from Jan 5, 2022

Conversation

acozzette
Copy link
Member

@acozzette acozzette commented Jan 5, 2022

Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.

@acozzette acozzette added java release notes: no Exclude this PR description in the next release kokoro:run labels Jan 5, 2022
@acozzette acozzette requested a review from elharo Jan 5, 2022
Copy link
Contributor

@elharo elharo left a comment

Not sure why the tests are failing. Has CI gone flaky again?

@@ -0,0 +1,78 @@
// Protocol Buffers - Google's data interchange format
// Copyright 2008 Google Inc. All rights reserved.
Copy link
Contributor

@elharo elharo Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2021 and otherwise update to current guidelines for copyright statements

Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.
Copy link
Contributor

@elharo elharo left a comment

Test failures look related:

++ '[' '!' -z 'java/core/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java ' ']'
++ echo 'Missing files in EXTRA_DIST: java/core/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java '
Missing files in EXTRA_DIST: java/core/src/test/java/com/google/protobuf/UnknownFieldSetPerformanceTest.java
++ exit 1

@acozzette
Copy link
Member Author

acozzette commented Jan 5, 2022

#9372 should fix the Ruby build failure if I understand the problem right.

elharo
elharo approved these changes Jan 5, 2022
@@ -232,6 +232,7 @@
<exclude>TypeRegistryTest.java</exclude>
<exclude>UnknownEnumValueTest.java</exclude>
<exclude>UnknownFieldSetLiteTest.java</exclude>
Copy link
Contributor

@elharo elharo Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not new, but why are we excluding the lite test here? Worth a look.

Copy link
Member Author

@acozzette acozzette Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good question. I'm not sure why.

@acozzette acozzette merged commit c555c3d into protocolbuffers:master Jan 5, 2022
52 checks passed
@acozzette acozzette deleted the java-unknown-fields branch Jan 5, 2022
acozzette added a commit to acozzette/protobuf that referenced this pull request Jan 5, 2022
…s#9371)

Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.
acozzette added a commit to acozzette/protobuf that referenced this pull request Jan 5, 2022
…s#9371)

Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.
acozzette added a commit to acozzette/protobuf that referenced this pull request Jan 5, 2022
…s#9371)

Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.
acozzette added a commit that referenced this pull request Jan 5, 2022
Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.
acozzette added a commit that referenced this pull request Jan 5, 2022
Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.
acozzette added a commit to acozzette/protobuf that referenced this pull request Jan 5, 2022
…s#9371)

Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.
acozzette added a commit to acozzette/protobuf that referenced this pull request Jan 5, 2022
…s#9371)

Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.
acozzette added a commit that referenced this pull request Jan 5, 2022
Credit should go to @elharo for most of these Java changes--I am just
cherry-picking them from our internal codebase. The one thing I did
change was to give the UTF-8 validation tests their own Bazel test
target. This makes it possible to give the other tests a shorter
timeout, which is important for UnknownFieldSetPerformanceTest in
particular.
@ydroneaud
Copy link

ydroneaud commented Feb 16, 2022

Is this fix going to be applied on 3.17.x branch ?

@acozzette
Copy link
Member Author

acozzette commented Feb 16, 2022

No, we're no longer maintaining the 3.17.x branch, so to get this fix you will have to upgrade to at least 3.18.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java release notes: no Exclude this PR description in the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants