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

CASSANDRA-18329: Upgrade Jamm #2488

Closed
wants to merge 4 commits into from
Closed

CASSANDRA-18329: Upgrade Jamm #2488

wants to merge 4 commits into from

Conversation

blerer
Copy link
Contributor

@blerer blerer commented Jul 13, 2023

Thanks for sending a pull request! Here are some tips if you're new here:

  • Ensure you have added or run the appropriate tests for your PR.
  • Be sure to keep the PR description updated to reflect all changes.
  • Write your PR title to summarize what this PR proposes.
  • If possible, provide a concise example to reproduce the issue for a faster review.
  • Read our contributor guidelines
  • If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

Copy link
Contributor

@ekaterinadimitrova2 ekaterinadimitrova2 left a comment

Choose a reason for hiding this comment

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

Review done. Overall it looks good; small questions and comments left. The main questions about InMemoryTries and the FullQueryLogger (Plus BinAuditLogger) were not updated here.
One thing that is not mentioned in the code comments but I discussed a few days ago with another community member - do we need to measure actually at all the ClusteringComparator CASSANDRA-16304? Now we can, as we fixed jamm for newer versions and hidden classes (which are used to implement lambdas), but should we measure this particular lambda?

I did not review the eclipse warnings commit as I believe it is out of scope for this ticket.
Perhaps you should mention it on CASSANDRA-18239, where we replace eclipse warnings with CheckerFramework.

@ekaterinadimitrova2
Copy link
Contributor

ekaterinadimitrova2 commented Jul 15, 2023

Almost forgot:

  1. Fixed test here:
    ekaterinadimitrova2@086800c
    (without this, the branch will not compile)
  2. Hacked jamm jar in this branch so we can test in CI (we won't commit it as part of cassandra/lib, this was only for test purposes until jamm 0.4.0 is released in maven): https://github.com/ekaterinadimitrova2/cassandra/tree/repeated-run2
  3. We also need to add jamm version update to 0.4.0 in:
    build.xml, redhat/cassandra.in.sh, conf/cassandra-env.sh, bin/cassandra.in.sh
  4. CI run with JDK11 and JDK17 shows that all known jamm-related failures are fixed with the new jamm version, including the flakiness from CASSANDRA-17884.
    The remaining Java 17 failures are already addressed in ECJ, netty update, and CASSANDRA-18180 (ready to commit when we drop JDK8).
    CI runs:
    JDK11 - https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/2418/workflows/e2a63ad4-4d51-45c0-a876-a45512b20073
    JDK17 - https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/2418/workflows/c386187e-dd3d-4e42-83c6-9602dd6759f1

Copy link
Contributor

@ekaterinadimitrova2 ekaterinadimitrova2 left a comment

Choose a reason for hiding this comment

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

There is a valid point that JDK8 is sill not dropped. Even if it will be dropped immediately after this ticket, it is worth it to confirm that test run too as jamm works with JDK8.
Started CI run here:
https://app.circleci.com/pipelines/github/ekaterinadimitrova2/cassandra/2422/workflows/ceb46a2f-c682-4830-bb14-3b0d75294f4a

@ekaterinadimitrova2
Copy link
Contributor

I mentioned it in Slack last week, but also adding here so it doesn't get lost.
We use slice() in BufferPool in allocateDirectAligned.

@blerer
Copy link
Contributor Author

blerer commented Jul 21, 2023

The issue with allocateDirectAligned is that it is not what we call a SLAB but we will also not measure it correctly, we will slightly underestimate the size by the shallow size of a direct Buffer. I do not think that we can do much about it.

@blerer blerer changed the title CASSNDRA-18329: Upgrade Jamm CASSANDRA-18329: Upgrade Jamm Jul 21, 2023
@ekaterinadimitrova2
Copy link
Contributor

The issue with allocateDirectAligned is that it is not what we call a SLAB but we will also not measure it correctly, we will slightly underestimate the size by the shallow size of a direct Buffer. I do not think that we can do much about it.

I do believe the current patch just mimicked the same behavior we already have. Even if we decide to improve anything around that, it would be in a different ticket as an improvement.

Copy link
Contributor

@ekaterinadimitrova2 ekaterinadimitrova2 left a comment

Choose a reason for hiding this comment

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

+1 ,just a few nits to be adressed on commit. All test results were published on the ticket and LGTM

@@ -98,6 +99,11 @@ public void log(AuditLogEntry auditLogEntry)
@VisibleForTesting
public static class Message extends BinLog.ReleaseableWriteMarshallable implements WeightedQueue.Weighable
{
/**
* The shallow size of a {@code Query} object.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The shallow size of a {@code Message} object.

*/
private static final long EMPTY_SIZE = ObjectSizes.measure(new Batch());

/**
* The way is pre-computed in the constructor and represent the object deep size.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The weight is pre-computed in the constructor and represents the object's deep size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants