<div dir="ltr">Hi Karen,<div><br></div><div>Fair enough and thanks for your help :) First time I ask for a GC change :)</div><div><br></div><div>So exactly: this is not a final review at all. I provide the webrev so people can see what it looks like and that there is no confusion. Here are some additional points:</div><div><br></div><div>- It really is not the final review since as you say, last time I had a change that modified various architectures, I got help from different people to do the testing on the different architectures that were affected.</div><div><br></div><div>- To answer the graal question, technically there are two graal changes required:</div><div>   - A first one is due to this change, where the field name change will be needed to be done on the graal side; and that is relevant to this change <span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">8201326</span></div><div>   - Second is the fact Graal still does a FastTlabRefill and should not; that is related to graal change <span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">8171119; I had opened <a href="https://bugs.openjdk.java.net/browse/GRAAL-64?filter=-1">GRAAL-64</a> for that change</span></div><div><span style="color:rgb(34,34,34);font-family:sans-serif;font-size:13px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div>So therefore, the question becomes a two-parter:</div><div>  - Would someone be willing to sponsor this change for me and help with the tests?<br></div><div>  - Would that person and/or others able to perform the testing for the various architectures want to help run a set of tests on the change?</div><div><br></div><div>Then, if people do agree to the design of the change and I can get a sponsor, I can move this after initial testing (or after full testing) to the main hotspot-dev email list.</div><div><br></div><div>Thanks again for all your help!</div><div>Jc</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Apr 9, 2018 at 1:16 PM Karen Kinnear <<a href="mailto:karen.kinnear@oracle.com">karen.kinnear@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space">JC,<div><br></div><div>So I am the one who suggested that you ask the GC folks if they were ok with name change going in in advance,</div><div>since the merging includes a number of files in a rapidly changing repository. Thank you for pointing out the risks.</div><div><br></div><div>I am going to assume that this is a review for the approach, not the final source code review because:</div><div>- I do not see the full set of tests run - which you would coordinate with your sponsor</div><div><br></div><div>If the GC team is ok with the approach and you have all the tests passing - please send the actual code review request to</div><div><a href="mailto:hotspot-dev@openjdk.java.net" target="_blank">hotspot-dev@openjdk.java.net</a>. We use the team aliases for design consulting. We use the larger alias before anything goes in.</div><div><br></div><div>See below ...</div><div><div><br><blockquote type="cite"><div>On Apr 9, 2018, at 1:24 PM, JC Beyler <<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>> wrote:</div><br class="m_-453713170275573719Apple-interchange-newline"><div><div dir="ltr">Hi all,<div><br></div><div>Small pre-amble to this request:</div><div>In my work to try to get a heap sampler in OpenJDK (via <a href="https://bugs.openjdk.java.net/browse/JDK-8171119" target="_blank">JEP 331</a>), I'm trying to reduce the footprint of my change so that the integration can be easier. I was told that generally a JEP webrev should be feature complete and go in at-once. However, with the change touching quite a bit of various code pieces, I was trying to figure out what could be separated as not "part of the feature".</div><div><br></div><div>I asked around and said that perhaps a solution would be to cut up the renaming of TLAB's end field that I do in that webrev. Because I'm renaming a field in TLAB used by various backends for that work, I have to update every architecture dependent code to reflect it.</div><div><br></div><div>I entirely understand that perhaps this is not in the habits and very potentially might not be the way things are generally done. If so, I apologize and let me know if you would not want this to go in separately :)</div><div><br></div><div>Final note: there is still a chance JEP-331 does not go in. If it does not, we can leave the new name in place or I'll happily revert it. I can even create an issue to track this if that makes it easier for all.</div><div><br></div><div>End of the pre-amble.</div><div><br></div><div><br></div><div>The 33-line change webrev in question is here:<br></div><div><a href="http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.00/" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.00/</a><br></div><div><br></div><div>I fixed all the architectures and JVMCI and ran a few sanity tests to ensure I had not missed anything.</div><div><br></div><div>Thanks for your help and I hope this is not too much trouble,</div><div>Jc</div><div><br></div><div>Ps: there is a graal change that needs to happen but I was not sure <a href="https://teams.googleplex.com/u/where" target="_blank">who/where</a> to ask about it. I was told it could happen in a separate webrev. Can anyone point me to the right direction? Should it just be hotspot-compiler-dev?</div></div>
</div></blockquote>Can I assume the graal change is not related to 8201326, but part of the 8171119 heap sampler collection email thread? That already</div><div>includes the compiler team, so you should be set there.</div><div><br></div><div>thanks,</div><div>Karen</div><br></div></div></blockquote></div>