[Truffle] returnIDs use Objects, Thread#raise, minor updates #3140

pitr-ch · 2015-07-15T18:52:30Z

No description provided.

eregon · 2015-07-16T08:38:23Z

truffle/src/main/ruby/core/rubinius/bootstrap/thread.rb

+      Truffle::Primitive.thread_raise self, exc
+    end
+  end
+


Where does this code come from? If it is modified it should be in api/shims.
If we can work with the original code, it is usually better, so in this case we could implement the thread_raise Rubinius primitive, right? (In ThreadPrimitiveNodes) See VMPrimitiveNodes for an example of Rubinius primitives nodes.

The original Rubinius Thread#raise uses Rubinius.lock and other intrinsic @alive variable, checks if the Thread is alive etc. That's the reason why this method is modified, so moving to shims. Does it makes sense to use Rubinius primitive if the ruby wrapping code is different?

Yeah, I think having the Rubinius primitive still makes sense to preserve most of the original code (and also better organization, not visible so directly to user, etc).

eregon · 2015-07-16T08:40:12Z

Looks great, please address my 3 comments.

chrisseaton · 2015-07-16T12:27:18Z

truffle/src/main/java/org/jruby/truffle/nodes/core/TrufflePrimitiveNodes.java

+    @CoreMethod(names = "thread_raise", required = 2, onSingleton = true)
+    public abstract static class RaiseNode extends CoreMethodArrayArgumentsNode {
+
+


Please be careful about things like random extra newlines.

ok, IDE formatter configuration updated.

eregon · 2015-07-19T09:20:37Z

truffle/src/main/ruby/core/rubinius/api/shims/thread.rb

+      raise_prim exc
+    end
+  end
+


All of this should be in a separate file, because the loading order will load Rubinius' common/thread.rb after this according to core.rb.
The solution is to move it to a separate file, for instance named thread_common.rb or thread_after_common.rb and add that one after common/thread,rb in core.rb.
We should have a better organization for all of this.
cc @nirvdrum.

Small correction Thread#raise is defined in bootstrap/thread we do not have it defined in our repo though, which was to reason why I've putted into existing shims/thread. You are right that it's error prone (if we later copy the bootsrap) and it would be better to have it loaded in extra file. Moving.

Ah so that's why the CI passes :) Indeed, the more clarity the better!

nirvdrum · 2015-07-19T14:55:44Z

The comments were deleted when new changes were pushed, so I think I only have part of the conversation from email. But, if you want Thread.raise from the Rubinius bootstrap file (and don't need to change it), just import it from the correct version of Rubinius. The bootstrap files have been treated as additive, while the core files are 100% what upstream has and we exclude some stuff we can't use.

pitr-ch · 2015-07-20T06:07:45Z

@nirvdrum I've used just part of the original method, that's why the discussion where to put it. The original method used internal implementation details. I did not import the original bootstrap method, should I?

nirvdrum · 2015-07-20T12:19:20Z

@pitr-ch That's fine. If you find yourself needing to only exclude part of a method Truffle.omit is an option as well (grep for current usages).

nirvdrum · 2015-07-20T12:21:19Z

truffle/src/main/ruby/core/rubinius/api/shims/thread_common.rb

+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#
+# Modifications are subject to:


Unless you've added any code (i.e., didn't just delete stuff from Rubinius), you really don't need this. And even then, it's questionable for trivial changes. There's no harm in having it here, but it's not usually a requirement.

But large parts of both methods below are part of Rubinius, so the license should be in this file, no?

Yes. The Rubinius license should be in tact. I specifically meant the modification parts.

There are few minor additions, so I'll keep it in this case.

Another possible approach to this could be to always copy the original method with the rubinius license and have the overrides in shims with truffle license. Then it would not be mixed and we would be able to say easily what is the difference. What do you think?

nirvdrum · 2015-07-20T12:22:31Z

FYI, I think what you did with the new shims file is fine. As @eregon said, now that we have a pattern of a few use cases, we should tidy up. Most of the files in api/shims really aren't shims for an API.

eregon · 2015-07-20T14:00:43Z

@nirvdrum Yeah, I think actually most of "API shims" are really just defined in bootstrap in proper Rubinius and it would make more sense to define it with our files at that same bootstrap stage.

nirvdrum · 2015-07-20T14:20:08Z

truffle/src/main/ruby/core/rubinius/api/shims/thread_common.rb

+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#
+# Modifications are subject to:
+# Copyright (c) 2014, 2015 Oracle and/or its affiliates. All rights reserved. This


Unless you ported this from elsewhere, the code should not be copyrighted in 2014.

Good point it should be just 2015

pitr-ch · 2015-07-20T14:31:40Z

yeah, right now the core.rb serves as a source to find out where the file belongs. We could have shims/ followed by the same structure of directories as rubinus has for its files.

Regarding this, PR can I merge?

chrisseaton · 2015-07-20T14:41:01Z

Regarding this, PR can I merge?

Not today! We're waiting for the 9k release.

chrisseaton · 2015-07-22T18:40:35Z

@pitr-ch sorry couldn't merge this. Can you pull, fix and merge?

pitr-ch · 2015-07-23T08:32:57Z

@chrisseaton np, after ci passes i'll merge

It was giving bad rusults for cases like: {a:1, b:2}.merge(b:3) # => {:a=>1, :b=>2}

* to be able to get the backtrace in debuger

…e instance

from core/rubinius/boostrap/thread to core/rubinius/api/shims/thread

…::Primitive

…tive it should return Time instance only when called in class Time or a descendant class RSpec::Core::Time class << self define_method(:now, &::Time.method(:now)) end end RSpec::Core::Time.now was returning RSpec::Core::Time instence instead of Time

pitr-ch added the truffle label Jul 15, 2015

pitr-ch self-assigned this Jul 15, 2015

pitr-ch added this to the truffle-dev milestone Jul 15, 2015

eregon reviewed Jul 16, 2015
View reviewed changes

pitr-ch force-pushed the master branch from fe3bd7c to 32a7eb9 Compare July 16, 2015 09:05

chrisseaton reviewed Jul 16, 2015
View reviewed changes

pitr-ch force-pushed the master branch 3 times, most recently from 08d32c3 to 892f039 Compare July 18, 2015 12:29

eregon reviewed Jul 19, 2015
View reviewed changes

pitr-ch force-pushed the master branch from 892f039 to 232190d Compare July 19, 2015 10:07

pitr-ch force-pushed the master branch from adb0160 to c03fac5 Compare July 20, 2015 06:25

nirvdrum reviewed Jul 20, 2015
View reviewed changes

pitr-ch force-pushed the master branch from 603c91f to 6030cd8 Compare July 22, 2015 15:38

pitr-ch force-pushed the master branch from 6030cd8 to 483c4d5 Compare July 23, 2015 08:25

pitr-ch force-pushed the master branch from 483c4d5 to 9aee274 Compare July 23, 2015 13:24

pitr-ch added 2 commits July 23, 2015 18:06

Add specs to Hash#merge with small hashes

7d8c779

[Truffle] remove broken case from pack-pack Hash merge

90aede3

It was giving bad rusults for cases like: {a:1, b:2}.merge(b:3) # => {:a=>1, :b=>2}

pitr-ch added 17 commits July 23, 2015 18:06

[Truffle] Add printableBacktrace to DebugOperations

01c1e58

* to be able to get the backtrace in debuger

[Truffle] Do not fail when backtrace is not set

9f1e745

[Truffle] BigDecimal: rename create methods to initialize if they tak…

41a8a10

…e instance

[Truffle] Thread#raise migrated to primitive

afc6ef2

[Truffle] Thread#raise add backtrace when missing

f7107f7

add test for backtrace presence

9db38b9

[Truffle] tag itermitent failure (hangs)

8930cdd

[Truffle] use new ReturnID object for returnID to avoid collisons

03ec251

[Truffle] code formatting

a4394db

[Truffle] move our Thread#raise implementation

2fb0d1c

from core/rubinius/boostrap/thread to core/rubinius/api/shims/thread

[Truffle] Better error message for rubinius UndefinedPrimitive

d5271b3

[Truffle] use Rubinius primitives for thread_raise instead of Truffle…

486aa2d

…::Primitive

[Truffle] Do not mix thread's before and after common methods

3155ce1

[Truffle] Fix license years

e6abeaa

Time#now should return Time instance when called on non-descendant class

3b37fd3

[Truffle] allow null exceptions in backtrace formatters as before

f38195d

pitr-ch force-pushed the master branch from 9470ebb to f38195d Compare July 23, 2015 16:06

pitr-ch merged commit f38195d into jruby:master Jul 23, 2015

enebo added this to the Non-Release milestone Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

[Truffle] returnIDs use Objects, Thread#raise, minor updates #3140

[Truffle] returnIDs use Objects, Thread#raise, minor updates #3140

pitr-ch commented Jul 15, 2015

eregon Jul 16, 2015

pitr-ch Jul 16, 2015

eregon Jul 17, 2015

pitr-ch Jul 18, 2015

pitr-ch Jul 18, 2015

eregon commented Jul 16, 2015

chrisseaton Jul 16, 2015

pitr-ch Jul 16, 2015

eregon Jul 19, 2015

pitr-ch Jul 19, 2015

eregon Jul 19, 2015

nirvdrum commented Jul 19, 2015

pitr-ch commented Jul 20, 2015

nirvdrum commented Jul 20, 2015

nirvdrum Jul 20, 2015

eregon Jul 20, 2015

nirvdrum Jul 20, 2015

pitr-ch Jul 20, 2015

nirvdrum commented Jul 20, 2015

eregon commented Jul 20, 2015

nirvdrum Jul 20, 2015

pitr-ch Jul 20, 2015

pitr-ch commented Jul 20, 2015

chrisseaton commented Jul 20, 2015

chrisseaton commented Jul 22, 2015

pitr-ch commented Jul 23, 2015

		@CoreMethod(names = "thread_raise", required = 2, onSingleton = true)
		public abstract static class RaiseNode extends CoreMethodArrayArgumentsNode {

[Truffle] returnIDs use Objects, Thread#raise, minor updates #3140

[Truffle] returnIDs use Objects, Thread#raise, minor updates #3140

Conversation

pitr-ch commented Jul 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon commented Jul 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirvdrum commented Jul 19, 2015

pitr-ch commented Jul 20, 2015

nirvdrum commented Jul 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirvdrum commented Jul 20, 2015

eregon commented Jul 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitr-ch commented Jul 20, 2015

chrisseaton commented Jul 20, 2015

chrisseaton commented Jul 22, 2015

pitr-ch commented Jul 23, 2015