weixin_39903176
weixin_39903176
2020-11-21 20:07

WIP: Decouple TR_PrexArgInfo from TR_InlinerTracer

Move code from TR_InlinerTracer into TR_PrexArgInfo class so that TR_PrexArgInfo is no longer tightly couple to the TR_InlinerTracer class.

This should make the TR_PrexArgInfo class more reusable.

  • Move code from TR_InlinerTracer::dumpPrexArgInfo into the new method TR_PrexArgInfo::dumpTrace
  • Change TR_PrexArgInfo method signatures to accept a TR_LogTracer argument instead of a TR_InlinerTracer argument since TR_LogTracer is a less specialized class.

Note: this needs to be merged at the same time as https://github.com/eclipse/openj9/pull/8419

Fixes: https://github.com/eclipse/openj9/issues/7936

Signed-off-by: Ryan Shukla

该提问来源于开源项目:eclipse/omr

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享
  • 邀请回答

9条回答

  • weixin_39903176 weixin_39903176 5月前
    点赞 评论 复制链接分享
  • weixin_39600400 weixin_39600400 5月前

    Could the changes be staged to avoid the coordinated merge - eg add the new dump methods / fields, update OpenJ9 and then remove the deprecated versions? Coordinated merges are a pain so we try to avoid them where reasonably possible. If it isn't could you just sketch the problem in staging the change?

    点赞 评论 复制链接分享
  • weixin_39903176 weixin_39903176 5月前

    Could the changes be staged to avoid the coordinated merge - eg add the new dump methods / fields, update OpenJ9 and then remove the deprecated versions? Coordinated merges are a pain so we try to avoid them where reasonably possible. If it isn't could you just sketch the problem in staging the change?

    The problem I see is that the TR_PrexArgInfo methods (which I have changed the signature of) are declared in omr but implemented in openj9.

    The only way that I can think of to overload these methods with the new versions without changing openj9 would be to implement the new versions here. Then we would have the old methods implemented in openj9 and the new methods implemented here.

    There could very well be a better way that I haven't thought of. Please let me know if I missed something.

    点赞 评论 复制链接分享
  • weixin_39600400 weixin_39600400 5月前

    So one thought would be could you add the new methods and implementations in OMR and have them uncalled (leaving the current ones alone), change OpenJ9 to use the new methods and then remove the OMR methods?

    点赞 评论 复制链接分享
  • weixin_39903176 weixin_39903176 5月前

    So one thought would be could you add the new methods and implementations in OMR and have them uncalled (leaving the current ones alone), change OpenJ9 to use the new methods and then remove the OMR methods?

    I could implement the new methods here (in omr) and then update openj9 to call these new methods. However, in order to move the implementation of the new methods back into openj9, I think a coordinated merge would still be necessary to avoid compile errors due to having 2 implementations of each method. Would it be preferable to do the coordinated merge at this later stage?

    Alternately, if you think it make more sense to have these methods implemented in omr, then I could follow your suggestion except leave the implementations in omr and remove them from openj9 later.

    点赞 评论 复制链接分享
  • weixin_39600400 weixin_39600400 5月前

    Let me have an offline discussion with you to work out details - I think this can most likely be staged and we can summarize our solution here (if we agree to one).

    点赞 评论 复制链接分享
  • weixin_39818662 weixin_39818662 5月前

    The change looks good other than 3 small things - coordinating with openj9 - needs to fix copyright - needs to squash into one commit

    点赞 评论 复制链接分享
  • weixin_39903176 weixin_39903176 5月前

    Summary of the solution Andrew and I discussed:

    1. Add the following macro in OMR PreExistence.hpp #define TR_PREXARGINFO_TRACER_CLASS TR_InlinerTracer Also add the new TR_PrexArgInfo::dumpTrace method.

    2. Update method definitions in openj9 to use that macro. Also code in the bodies of these methods to only use members from the TR_LogTracer class.

    3. Update OMR macro and method declarations to use TR_LogTracer

    4. Remove macro from openj9

    5. Remove macro from OMR

    点赞 评论 复制链接分享
  • weixin_39903176 weixin_39903176 5月前

    I'm going to close this PR and open a new one with the changes staged as described above.

    点赞 评论 复制链接分享

相关推荐