-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Add option to enable ir writing on-the-fly #3791
Conversation
the option and its desc are a little confusing - understand the need for a short desc but still unable to wrap my head around the option without reading the PR desc. |
11812b3
to
f2646ec
Compare
Option name changed. |
@chyh1990 I still don't like this option name. This option is really just recording or tracking lexical containment of scopes. This information can be used by more than dumping/persistence (for example, we use it when dry run is specified). Names are tough :) The essence is that it is recording/tracking lexical/static child scopes. Hmmm: ir.record_lexical_scopes. This is a little longer but it is already a JRUBY_OPTS sort of flag. @subbuss you have an opinion on this? @chyh1990 As for your extension, I would like to try and minimize how much persistence code you need to maintain on this. So far no one has depended on IR persistence outside of the core project so we have been much more readily breaking stuff. The actual interfaces for IR persistence has not changed as much but our instruction set is evolving and I fear us breaking you. In looking at your provided extension source one glaring issue is that our IRWriter is not extendable. Had we made those classes with non-static methods then you could have used a lot more of our original code. OR we could have made all these static methods public. I think the former is better than the later. Since you had to wrestle with how we organized this do you have any thoughts on improvement? I could fairly quickly make an instance-based version of IRWriter/IRReader (largely just make it an instance but still keep the public static methods for backwards compat. Then I think you will largely only need to override one method to add your static scope saving code. What do you think? |
Yes, dump_scopes is the not right flag. record_lexical_scopes is better but it doesn't quite capture that you are recording lexical nesting of scopes ... so, maybe record_lexical_hierarchy? |
@enebo Thanks for your reply! As you say, all the trouble comes from the non-extendable
do not handle
I definitely want to see improvement of closure serialization feature in Ruby. This feature is available in scala/python and even R but hardly exists in ruby world. @enebo is |
f2646ec
to
29fbd9e
Compare
change the option name to |
Hi,
I'm working on ruby (JRuby only) binding for Apache Spark. I have finished a working prototype which enables REAL closure serialization (IRClosure and variables in dynamic scopes) on JRuby:
https://github.com/chyh1990/jruby-spark/blob/master/src/main/java/com/sensetime/utils/ProcToBytesService.java
It's actually a java extension, which bring marshalling to
Proc
.But I found JRuby do not matain children static scopes by default.
Set
jruby.ir.writing
to true can solve this problem, but it will write out a lot of IR files. So I addan option to force children static scopes registration, which enable IR writing on the fly.
I think it will be amazing to enable closure serialization on JRuby, which makes it possible to write
a fully functional Ruby-Spark binding.