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

请帮忙提名OpenJDK Committer #21

Open
wenshao opened this issue Jul 26, 2024 · 10 comments
Open

请帮忙提名OpenJDK Committer #21

wenshao opened this issue Jul 26, 2024 · 10 comments

Comments

@wenshao
Copy link

wenshao commented Jul 26, 2024

@liach

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

@liach
Copy link
Member

liach commented Jul 26, 2024

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

这次 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
Copy link
Member

liach commented Jul 26, 2024

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

@wenshao
Copy link
Author

wenshao commented Jul 26, 2024

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

这次 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
Copy link
Author

wenshao commented Jul 27, 2024

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

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

我的领域包括:

  1. 性能优化(主要是字符串处理方面)
  2. JSON
  3. SQL,SQL Parser & JDBC

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

  • java.lang中,String Concat, StringBuilder, Integer parse & toString, Long parse & toString,Float.toString, Double.toString
  • java.util中的Date、HexFormat、UUID、Formatter
  • java.time parse & format & toString
  • java.math BigInteger & BigDecimal parse & toString

@liach
Copy link
Member

liach commented Jul 27, 2024

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

  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
Copy link
Author

wenshao commented Jul 27, 2024

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

  1. 核心库可以有很多地方可以优化性能,并且不影响兼容,当然这里需要做好维护性和性能的平衡。
  • 比如 PR #19626 ,最初的版本是使用Unsafe.putInt 加一个常数来实现appendNull和append(boolean),性能是很好的,但维护性不好。现在eme64正在优化C2达到同样的效果。这样的PR会整体提升JDK的性能,它的意义超过这个优化本身。

  • 有一些优化效果极好,但会带来复杂大,但平衡点在哪里,不同的项目接受程度不一样。通过这段时间的配合,我在尝试找到核心库小组的接受的平衡点。

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

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

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

@liach
Copy link
Member

liach commented Jul 27, 2024

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

一般核心库标准实现怕复杂度增加;毕竟是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
Copy link
Author

wenshao commented Jul 27, 2024

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

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

@liach
Copy link
Member

liach commented Jul 27, 2024

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

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

@wenshao
Copy link
Author

wenshao commented Aug 21, 2024

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

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

No branches or pull requests

2 participants