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 paths to *DynamicScope that don't check nulls, offsets. #4168

Merged
merged 2 commits into from Sep 22, 2016

Conversation

Projects
None yet
1 participant
@headius
Member

headius commented Sep 22, 2016

Our IR performes liveness checks on variables and should never
emit code that accesses variable indices outside the prescribed
scope size. This patch adds new getters that do not perform these
unnecessary checks, and wires up the JIT to call them.

This patch does not modify Full interpretation to use these
unchecked paths, because it would require either a boolean in
LocalVariaable operand and LoadLocalVariable instruction or a
pass to replace them with "Unchecked" versions.

It would also be better if we could generate these shapes of
DynamicScope on the fly, rather than maintaining a finite set of
mostly-identical subclasses.

See #4167.

Add paths to *DynamicScope that don't check nulls, offsets.
Our IR performes liveness checks on variables and should never
emit code that accesses variable indices outside the prescribed
scope size. This patch adds new getters that do not perform these
unnecessary checks, and wires up the JIT to call them.

This patch does not modify Full interpretation to use these
unchecked paths, because it would require either a boolean in
LocalVariaable operand and LoadLocalVariable instruction or a
pass to replace them with "Unchecked" versions.

It would also be better if we could generate these shapes of
DynamicScope on the fly, rather than maintaining a finite set of
mostly-identical subclasses.

See #4167.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 22, 2016

Member

The one JIT failure appears to be in this method; the "object" variable here is coming back as null. Why?

lib/ruby/stdlib/test/unit/assertions.rb:393

      def assert_nil(object, message="")
        full_message = build_message(message, <<EOT, object)
<?> expected to be nil.
EOT
        assert_block(full_message) { object.nil? } # this nil? call fails
      end
Member

headius commented Sep 22, 2016

The one JIT failure appears to be in this method; the "object" variable here is coming back as null. Why?

lib/ruby/stdlib/test/unit/assertions.rb:393

      def assert_nil(object, message="")
        full_message = build_message(message, <<EOT, object)
<?> expected to be nil.
EOT
        assert_block(full_message) { object.nil? } # this nil? call fails
      end
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 22, 2016

Member

Ok I think the error is because Thread#name is not initialized to any value. It's returning nulls into Ruby!

Pushing a fix shortly.

Member

headius commented Sep 22, 2016

Ok I think the error is because Thread#name is not initialized to any value. It's returning nulls into Ruby!

Pushing a fix shortly.

Initialize Thread name to nil. It was returning null if unset.
This was returning null into Ruby for any thread that did not
have a name, which could cause NullPointerException far away from
the point of call. It was masked in some cases by our
DynamicScope logic that checks for null values, but #4168 removes
those checks for the JIT.

@headius headius added this to the JRuby 9.1.6.0 milestone Sep 22, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 22, 2016

Member

At least the Thread#name fix needs to get into 9.1.6.0.

Member

headius commented Sep 22, 2016

At least the Thread#name fix needs to get into 9.1.6.0.

@headius headius merged commit 7424803 into jruby:master Sep 22, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@headius headius deleted the headius:unchecked_dynamicscope branch Sep 22, 2016

enebo added a commit that referenced this pull request Sep 28, 2016

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