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

Consider switching JSR223 engine to use "persistent" local variables #5012

Closed
headius opened this Issue Jan 26, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@headius
Member

headius commented Jan 26, 2018

In #1952 we learned from @mslinn that our JSR223 engine does not persiste variable updates across script executions, while most of the other languages do.

They provided a suite of scripting tests with some JRuby assertions commented out because they failed to behave as expected. I was able to confirm this issue in latest JRuby 9.1 HEAD.

By enabing on perisistent local variables (org.jruby.embed.localvariable.behavior=persistent), all of the commented assertions passed.

So there may be a good reason to switch JSR223 to use persistent local variables. I am reluctant to just do it since it means those variables will now be retained across calls, and previous calls' variables will be visible to subsequent calls. Both are fairly visible changes if anyone out there is depending on these variables to be clean each time.

@headius headius added this to the JRuby 9.2.0.0 milestone Jan 26, 2018

@headius

This comment has been minimized.

Member

headius commented Jan 26, 2018

Given the impact I think 9.2 is the earliest we could do this.

@headius

This comment has been minimized.

Member

headius commented Jan 29, 2018

@kares @enebo @mkristian Any opinions on this?

@enebo

This comment has been minimized.

Member

enebo commented Jan 29, 2018

I remember the author (A. Sund??????) of the original (and I do not think is our) JSR223 impl made some decisions about what would persist across calls. He did most of the original bindings for most of the languages (Sun employee). I was fairly sure we copied those decisions.

If I ignore that then the obvious reason this is a bad idea is it will pin old objects over time if you keep adding new lvars. If your script is ('foo = 2 unless foo') then you likely will either a) be exploiting this feature or b) going off in the weeds since you did not realize foo had been used by another call previously. With that said, there is also the added confusion that the engine keeps constants/methods/globals loaded. So that is inconsistent. If I remember @Ivars will also persist across invocations? Of all the things which could persist I think local variables are probably the most likely to be unexpected. They also have no API for de-allocating the slot in the scope. You can uninitialize if you use 'local_variables' to get a list and set to nil but the scope table will just grow forever.

With my long rambling personal opinion stated if all the other languages do this then perhaps we should too. We will be exchanging issue reports on being inconsistent with leaking memory (and changed behavior). Since we have probably had ~5 jsd223 bugs in the last 10 years we can probably go either way :)

We should also ask @yokolet what she thinks. She may remember more of the history of JSR223.

@headius

This comment has been minimized.

Member

headius commented Feb 13, 2018

@enebo I am currently leaning toward just making this change. I have no idea how many people are using JRuby + JSR223, but I can't imagine there's many. Most folks who want to embed JRuby want the richer API, and that's what we have usually told people asking about embedding.

If you agree this would be simple to drop into 9.1.16, and see if anyone hates it.

@headius headius closed this in 91278e7 Feb 26, 2018

@headius

This comment has been minimized.

Member

headius commented Feb 26, 2018

The change is made, and only for 9.2+ because of a regression it can cause...

The old behavior was "global", which meant that local variables "put" into the engine went into $variable style names. This meant they were persistent, but this did not match the behavior of other language engines that simply use normal local variable names.

By switching the "persistent" we gain the persistent local variables, but lose the global variable. This will break code that expected the global variable.

Given the fact that we've had at least two JSR-223 engine users report that variables do not persist like they expected, we are making the change for 9.2. It can be switched back to "global" or to some other mode by setting the JVM property org.jruby.embed.localvariable.behavior=global.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment