search

liachmodded / talk

The comment section for user @liach
https://github.com/liachmodded/talk
1 stars 0 forks source link

请帮忙提名OpenJDK Committer #21

Open wenshao opened 3 months ago

wenshao commented 3 months ago

@liach

https://github.com/openjdk/jdk/pulls?q=is%3Apr+author%3Awenshao+is%3Aclosed+label%3Aintegrated 我提交给OpenJDK被合入的PR已经22个了，你是否可以帮我提名Committer？

liach commented 3 months ago

现在补丁和以前的比的确更好了，不像以前改改改超界，加一堆难维护的代码这样的。现在优化的确简洁明了，带来的代码简洁提升维护简易度的价值甚至大于优化的性能价值。

这次 openjdk/jdk#20321 我也犯错了，意识到注释也需要看看，经常注释写出来别人评论才发现对代码的理解有误。所以以后我们最好/integrate或者你成为commiter后/sponsor合并要在最后提交（不止是代码提交，注释提交也算）后过个20小时这样，大家才有机会都看一下发现问题。同时有些补丁跨领域，虽然标"ready"但是还是要别的领域的reviewer看下说没问题才能合并。我也要学习这些点。

我问了下别的committer，他们感觉因为你是弄优化的，更适合claes这样的专业优化人员提名。我个人偏功能性内容，而且资历偏低，有点跨领域，主要弄reflection和classfile api。同时因为你的patch一般是你自己开ticket加"performance optimization"，但是openjdk的角色是reference implementation（更加关注specification和behavior，官方标准），一般认为价值没有修那些影响行为的JBS上已知的bug（比如你发现然后修的openjdk/jdk#16033）高。然后有几个之前的补丁是补丁有问题擦屁股（比如openjdk/jdk#14745），这种一般也不算significant。

大家推荐你继续发 openjdk/jdk#16033 这样修规范和行为漏洞和 openjdk/jdk#20322 这样清理维护的补丁；一般JDK本身的性能提升是作为清理维护附带作用，如果库实在追求性能一般推荐直接用byte[]或者ByteBuffer。

liach commented 3 months ago

@wenshao 你现在发的补丁太多了，大家review不过来了。你说还有很多优化要提交补丁，这个我推荐你这样处理，暂时不急着开一堆补丁；搞个文档（比如我这里开个issue）记录所有优化，过段时间积累一些，然后再看怎么分补丁。 1、同一种改动，比如改用StringBuilder.repeat不需要每个class一个补丁；甚至整个java.base模块一个补丁就够了 2、这样分补丁虽然总量变大了，但是reviewer对总体目的和改动更明确，review起来更快，而且一次性通过的优化更多，同时你pull request因为目的明确，提交后一般根据要求做的改动也更少。

wenshao commented 3 months ago

现在补丁和以前的比的确更好了，不像以前改改改超界，加一堆难维护的代码这样的。现在优化的确简洁明了，带来的代码简洁提升维护简易度的价值甚至大于优化的性能价值。

这次 openjdk/jdk#20321 我也犯错了，意识到注释也需要看看，经常注释写出来别人评论才发现对代码的理解有误。所以以后我们最好/integrate或者你成为commiter后/sponsor合并要在最后提交（不止是代码提交，注释提交也算）后过个20小时这样，大家才有机会都看一下发现问题。同时有些补丁跨领域，虽然标"ready"但是还是要别的领域的reviewer看下说没问题才能合并。我也要学习这些点。

我问了下别的committer，他们感觉因为你是弄优化的，更适合claes这样的专业优化人员提名。我个人偏功能性内容，而且资历偏低，有点跨领域，主要弄reflection和classfile api。同时因为你的patch一般是你自己开ticket加"performance optimization"，但是openjdk的角色是reference implementation（更加关注specification和behavior，官方标准），一般认为价值没有修那些影响行为的JBS上已知的bug（比如你发现然后修的openjdk/jdk#16033）高。然后有几个之前的补丁是补丁有问题擦屁股（比如openjdk/jdk#14745），这种一般也不算significant。

大家推荐你继续发 openjdk/jdk#16033 这样修规范和行为漏洞和 openjdk/jdk#20322 这样清理维护的补丁；一般JDK本身的性能提升是作为清理维护附带作用，如果库实在追求性能一般推荐直接用byte[]或者ByteBuffer。

  1. 感谢你的建议，我以后会更关注代码的可维护性和性能优化的平衡，这个确实很重要，也是我参与以来学习到最重要的能力。

  2. 我以后会严格遵循24小时后integrate的规则

  3. claes的提名更合适的。你也很合适，你给我的评论很多，帮助也很多，感谢！！！怎么联系你和claes啊？使用openjdk的邮箱么？现在的PR是不是还不够啊，是否要再等一段时间再找claes提名。

  4. 如果提交的PR处理不过来，可以慢一些处理，慢下来对质量会更好，也容易得到更多人和更充分的讨论。提交大的PR合入会比较困难，我还是倾向于分开提交的小的PR，这样小步迭代，对每次Reviewer来说也容易一些，退一步来说，万一发现有问题，回滚处理也容易一些。

wenshao commented 3 months ago

@wenshao 你现在发的补丁太多了，大家review不过来了。你说还有很多优化要提交补丁，这个我推荐你这样处理，暂时不急着开一堆补丁；搞个文档（比如我这里开个issue）记录所有优化，过段时间积累一些，然后再看怎么分补丁。 1、同一种改动，比如改用StringBuilder.repeat不需要每个class一个补丁；甚至整个java.base模块一个补丁就够了 2、这样分补丁虽然总量变大了，但是reviewer对总体目的和改动更明确，review起来更快，而且一次性通过的优化更多，同时你pull request因为目的明确，提交后一般根据要求做的改动也更少。

我是以下开源项目的作者：

我的领域包括：

  1. 性能优化（主要是字符串处理方面）
  2. JSON
  3. SQL，SQL Parser & JDBC

最近一年参与OpenJDK，是希望把我字符串处理相关的经验技巧贡献到OpenJDK项目来，我希望在如下方面做贡献:

liach commented 3 months ago

你水平极高，学习能力更是惊人，一下子就会写字节码了。不过在字符串处理方面，核心库这边看法是这样的：

  1. JDK要支持的情况太多同时有特定的报错机制，很多情况无法优化，String 为了兼容加通用也是优化瓶颈

    • 比如 fastjson 里面本身就对常用字符集处理避免创造额外字符串了。那么这样似乎已经满足优化int long反序列化的性能需求了；理论上把这些反序列化的基础元素给其他object构造应该就行了，好像没理由再来优化 Integer.parseInt 了

  2. Date HexFormat UUID Formatter JSR310 都是i18n类的人维护，现在他们看优化有点看不过来了……

    • 你同一类的优化可以多改几个类，这样补丁数少点。虽然补丁代码量会变多，但是多这么一点点代码量不算问题，尤其这些代码改动的思路相同，不会带来额外的理解难处，方便review
    • 有些优化可以像 String concatenation 一样运行时生成字节码，比如 DateTimeFormatter 理论可以直接生成MethodHandle 专门处理 toString；我们因为人力资源有限同时还要进行其他新内容开发，所以一般希望能直接使用新内容新方法（比如我这里提到的 MethodHandle 就比较新）
    • 虽然现在 String Template 暂时撤了，主要原因是负责人退休了……同时好像要解决template呼叫时的method type的问题，支持method handle直连获得String concatenation类似性能。还是人手不够，JDK 25左右可能会回归。

  3. BigInteger BigDecimal：这两个感觉数字处理加速和避免复制数组算两类优化，可以考虑每类优化同时优化多个类型，比如biginteger String处理里面 group = val.substring() 可以改用 Java 9+ Integer.parseInt(CharSequence, int, int, ...) 的版本避免复制，然后BigDecimal用charsequence避免复制数组，归为同类补丁。（数位处理加速这个争议性比较大，但是如果达成共识后决定替换，一次也可以替换全部用途，所以一个pr够了）

怎么联系你和claes啊？使用openjdk的邮箱么？现在的PR是不是还不够啊，是否要再等一段时间再找claes提名。

是, openjdk邮箱可以联系，但是这个地址相当于中转，信会从openjdk关联邮箱收到。census里面用户名。

然后还有一点忘了提了：大家看到优化不知道起因。比如他们 test failure 改代码，会说 Xxx test failed 所以打补丁；你提交性能优化也是同理，说在什么大环境和实际应用中性能是问题，补丁带来了性能提升。同时benchmark也只是特定应用情况下的性能分析，所以benchmark的说服力经常没那么高。

wenshao commented 3 months ago

我在fastjson用到了asm bytecode，在classfile新提供的API类似，切换过来很容易。

  1. 核心库可以有很多地方可以优化性能，并且不影响兼容，当然这里需要做好维护性和性能的平衡。

  1. java.time里面优化的地方很多，如果做大的改动，维护小组是否能足够的时间处理，我最近尝试提交一些改进，也是看在维护小组的风格。

  2. Formatter可以做相关的优化很多，修改类本身是一种做法，在JavaCompiler AST层面做处理也是一种做法。我目前的感受是，核心小组不太愿意接受大的改变，这一点我想明确知道核心库小组的态度，后续才能在这方面继续投入。比如在PR #19926 中，claes说同意在String.format中做修改，这样会更广泛的效果，才会有 PR #19956 和 PR #20055 ，但感受到 RogerRiggs 的观点和claes是不一致的。

  3. 我提交的优化，大多数根据以往经验看到代码想到要做的改进，要找到最初的场景不容易。

liach commented 3 months ago

有一些优化效果极好，但会带来复杂大

一般核心库标准实现怕复杂度增加；毕竟是reference implementation，主要保证行为正确（所以我们改javadoc算改定义，要经过csr“兼容性和定义review”），稍微有复杂度的优化会让调整行为的代码改动更难。尤其是有hot path这种专用代码路径让调整行为正确和确保行为一致性难上加难，比如你的 PR 19956。

java.time里面优化的地方很多，如果做大的改动

如果是同类改动，大规模改动没什么影响：openjdk/jdk#4433 openjdk/jdk#4552

通过这段时间的配合，我在尝试找到核心库小组的接受的平衡点。 比如在PR #19926 中，claes说同意在String.format中做修改，这样会更广泛的效果，才会有 PR #19956 和 PR #20055 ，但感受到 RogerRiggs 的观点和claes是不一致的。

这个我解释一下：claes是专门负责性能优化的，但是rogerriggs、naotoj、我和大多数改动核心库的人是维护核心库本身（就是关心reference implementation的，甚至关心代码中留言的正确性，因为要便于理解和维护）同时还在进行新功能开发，比如rogerriggs现在在开发valhalla。所以我之前说如果你专心研究性能优化，claes提名更适合。

我个人认为你可以私下联系claes跟他分享推荐下你想准备的大优化，让他先确认下可以接受；比如字符串拼接这类是他负责和维护的，所以他对这类优化更开放。与此同时java.time和数字类现在没这类优化计划，开发人员也忙于其他项目。

感觉很多优化很可能你开个draft pr，这样讨论pr可以分享代码同时你就算经常提交也不会刷邮件，学习openjdk/jdk#19625，是一个JEP的样本代码，在大范围review前先有兴趣的人来看，然后初步看没问题后再开稳定pr。你未来的大型优化补丁也应该会这样，先claes和有空的人（类似我）看draft没问题后再开正式pr（一般draft里面评论历史太多，难翻到最新内容），减少给核心库区域维护者的review压力。

wenshao commented 3 months ago

java.time的相关优化，我尽量挑一些简单直接的改进，比如我新提的这个draft PR #20368 。我想看下他们开放性，再决定是否对DateTimeFormatter做大的改进，比如你说的基于MethodHandle或者ByteCode对DateTimeFormatter做优化。

claes应该是在准备JVMLS 2024的分享，我计划等他结束后再找他

liach commented 3 months ago

现在主要问题不是开放性，是你现在补丁数量就太多了，review不过来了

claes的准备内容在 https://cl4es.github.io/presentations/jvmls2024/draft.html 公开，实际上和你现在试验的东西很类似。可以联系一下他。

wenshao commented 2 months ago

https://mail.openjdk.org/pipermail/jdk-dev/2024-August/009326.html 求投票，谢谢