You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "JinwooHwang-SAS (via GitHub)" <gi...@apache.org> on 2025/07/13 16:32:29 UTC
[PR] ObjectSizerJUnitTest [geode]
JinwooHwang-SAS opened a new pull request, #7905:
URL: https://github.com/apache/geode/pull/7905
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [x] Does `gradlew build` run cleanly?
- [x] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "JinwooHwang-SAS (via GitHub)" <gi...@apache.org>.
JinwooHwang-SAS commented on code in PR #7905:
URL: https://github.com/apache/geode/pull/7905#discussion_r2282298847
##########
geode-core/src/test/java/org/apache/geode/internal/size/ObjectSizerJUnitTest.java:
##########
@@ -33,13 +34,23 @@ public void test() throws Exception {
assertEquals(roundup(OBJECT_SIZE), ObjectGraphSizer.size(new TestObject3()));
assertEquals(roundup(OBJECT_SIZE * 2 + REFERENCE_SIZE),
ObjectGraphSizer.size(new TestObject3(), true));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE), ObjectGraphSizer.size(new TestObject4()));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE) + roundup(OBJECT_SIZE + 4),
+ if (SystemUtils.isAzulJVM()) {
+ assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE + 8), ObjectGraphSizer.size(new TestObject4()));
Review Comment:
The size is different on Azul and 8 had to be added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "JinwooHwang-SAS (via GitHub)" <gi...@apache.org>.
JinwooHwang-SAS commented on PR #7905:
URL: https://github.com/apache/geode/pull/7905#issuecomment-3227551170
@raboof , I apologize for self-merging in this case — I realize now that it was not the right approach, particularly while there was still open feedback. The reason I proceeded was that I hadn’t received any feedback after my last comment, but I understand that I should have requested reviews more explicitly and waited. I truly appreciate the time and effort that goes into providing review feedback, and I’ll make sure to follow the proper process and reach out on Slack or the dev list before merging in the future. Please feel free to undo the merge if you find this is not the correct solution. Thank you for calling this out and helping me improve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on PR #7905:
URL: https://github.com/apache/geode/pull/7905#issuecomment-3193522319
> Can you explain the motivation for this change?
Ah I guess this is to make it succeed on Azul
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "JinwooHwang-SAS (via GitHub)" <gi...@apache.org>.
JinwooHwang-SAS commented on code in PR #7905:
URL: https://github.com/apache/geode/pull/7905#discussion_r2290608467
##########
geode-core/src/test/java/org/apache/geode/internal/size/ObjectSizerJUnitTest.java:
##########
@@ -33,13 +34,23 @@ public void test() throws Exception {
assertEquals(roundup(OBJECT_SIZE), ObjectGraphSizer.size(new TestObject3()));
assertEquals(roundup(OBJECT_SIZE * 2 + REFERENCE_SIZE),
ObjectGraphSizer.size(new TestObject3(), true));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE), ObjectGraphSizer.size(new TestObject4()));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE) + roundup(OBJECT_SIZE + 4),
+ if (SystemUtils.isAzulJVM()) {
+ assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE + 8), ObjectGraphSizer.size(new TestObject4()));
Review Comment:
As you can see in lines 35-36, OBJECT_SIZE and REFERENCE_SIZE are consistent whether it's Azul or not: assertEquals(roundup(OBJECT_SIZE * 2 + REFERENCE_SIZE),
ObjectGraphSizer.size(new TestObject3(), true));
That's why we cannot change those for Azul.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "JinwooHwang-SAS (via GitHub)" <gi...@apache.org>.
JinwooHwang-SAS commented on code in PR #7905:
URL: https://github.com/apache/geode/pull/7905#discussion_r2285457309
##########
geode-core/src/test/java/org/apache/geode/internal/size/ObjectSizerJUnitTest.java:
##########
@@ -33,13 +34,23 @@ public void test() throws Exception {
assertEquals(roundup(OBJECT_SIZE), ObjectGraphSizer.size(new TestObject3()));
assertEquals(roundup(OBJECT_SIZE * 2 + REFERENCE_SIZE),
ObjectGraphSizer.size(new TestObject3(), true));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE), ObjectGraphSizer.size(new TestObject4()));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE) + roundup(OBJECT_SIZE + 4),
+ if (SystemUtils.isAzulJVM()) {
+ assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE + 8), ObjectGraphSizer.size(new TestObject4()));
Review Comment:
We cannot change OBJECT_SIZE or REFERENCE_SIZE just for Azul because they are also used for other JVM implementations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #7905:
URL: https://github.com/apache/geode/pull/7905#discussion_r2280329671
##########
geode-core/src/test/java/org/apache/geode/internal/size/ObjectSizerJUnitTest.java:
##########
@@ -33,13 +34,23 @@ public void test() throws Exception {
assertEquals(roundup(OBJECT_SIZE), ObjectGraphSizer.size(new TestObject3()));
assertEquals(roundup(OBJECT_SIZE * 2 + REFERENCE_SIZE),
ObjectGraphSizer.size(new TestObject3(), true));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE), ObjectGraphSizer.size(new TestObject4()));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE) + roundup(OBJECT_SIZE + 4),
+ if (SystemUtils.isAzulJVM()) {
+ assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE + 8), ObjectGraphSizer.size(new TestObject4()));
Review Comment:
This '+ 8' seems suspicious to me - doesn't this indicate that either `OBJECT_SIZE`, `REFERENCE_SIZE` or `ObjectGraphSizer.size` needs to be different on Azul?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on PR #7905:
URL: https://github.com/apache/geode/pull/7905#issuecomment-3227589208
Thanks ;).
I won't revert the change, but I still don't find `assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE + 8), ObjectGraphSizer.size(new TestObject4()));` a meaningful assertion - it's clear _that_ the size is `roundup(OBJECT_SIZE + REFERENCE_SIZE + 8)`, but it's not clear _why_. Unless we can motivate why it really _should_ be `roundup(OBJECT_SIZE + REFERENCE_SIZE + 8)` I think I'd have preferred to just wrap the assertions that don't hold on Azul in a `if (!SystemUtils.isAzulJVM()) {` without making statements about the azul scenario. OK for now though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #7905:
URL: https://github.com/apache/geode/pull/7905#discussion_r2285555438
##########
geode-core/src/test/java/org/apache/geode/internal/size/ObjectSizerJUnitTest.java:
##########
@@ -33,13 +34,23 @@ public void test() throws Exception {
assertEquals(roundup(OBJECT_SIZE), ObjectGraphSizer.size(new TestObject3()));
assertEquals(roundup(OBJECT_SIZE * 2 + REFERENCE_SIZE),
ObjectGraphSizer.size(new TestObject3(), true));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE), ObjectGraphSizer.size(new TestObject4()));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE) + roundup(OBJECT_SIZE + 4),
+ if (SystemUtils.isAzulJVM()) {
+ assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE + 8), ObjectGraphSizer.size(new TestObject4()));
Review Comment:
Right, but the initialization of those values could depend on `SystemUtils.isAzulJVM()`, right? And the fact that this test failed perhaps suggests it should?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on code in PR #7905:
URL: https://github.com/apache/geode/pull/7905#discussion_r2282370302
##########
geode-core/src/test/java/org/apache/geode/internal/size/ObjectSizerJUnitTest.java:
##########
@@ -33,13 +34,23 @@ public void test() throws Exception {
assertEquals(roundup(OBJECT_SIZE), ObjectGraphSizer.size(new TestObject3()));
assertEquals(roundup(OBJECT_SIZE * 2 + REFERENCE_SIZE),
ObjectGraphSizer.size(new TestObject3(), true));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE), ObjectGraphSizer.size(new TestObject4()));
- assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE) + roundup(OBJECT_SIZE + 4),
+ if (SystemUtils.isAzulJVM()) {
+ assertEquals(roundup(OBJECT_SIZE + REFERENCE_SIZE + 8), ObjectGraphSizer.size(new TestObject4()));
Review Comment:
but does it make sense to add it in the test? wouldn't it make more sense to add it to `OBJECT_SIZE` or `REFERENCE_SIZE`? Or is there something in the calculation that needs to be different?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "JinwooHwang-SAS (via GitHub)" <gi...@apache.org>.
JinwooHwang-SAS merged PR #7905:
URL: https://github.com/apache/geode/pull/7905
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "JinwooHwang-SAS (via GitHub)" <gi...@apache.org>.
JinwooHwang-SAS commented on PR #7905:
URL: https://github.com/apache/geode/pull/7905#issuecomment-3227636420
Thank you very much for the thoughtful feedback, @raboof. I fully agree that the assertion roundup(OBJECT_SIZE + REFERENCE_SIZE + 8) does not clearly communicate why this should be the expected value, and in hindsight it does not provide sufficient motivation for the Azul JVM scenario.
I made an effort to investigate why Azul’s JVM behaves differently here, but unfortunately I have not been able to determine the underlying reason. Because of that, I must admit I am not entirely satisfied with the current solution, and I share your concern that it may not be the most appropriate long-term approach. Wrapping the assertions does indeed seem like a cleaner and less speculative direction, and I would be glad to revisit and refine this in a follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on PR #7905:
URL: https://github.com/apache/geode/pull/7905#issuecomment-3226167056
Whoa, I get that you're eager to get contributions in, but self-merging when there's open review feedback is unexpected. Self-merging when there are no reviews is also not great. You could at least have asked for reviews on the PRs, on Slack or on the dev list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ObjectSizerJUnitTest [geode]
Posted by "raboof (via GitHub)" <gi...@apache.org>.
raboof commented on PR #7905:
URL: https://github.com/apache/geode/pull/7905#issuecomment-3193521053
`:geode-core:spotlessJavaCheck` is failing for this PR.
Can you explain the motivation for this change? Doesn't this test seem to succeed on the main branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org