[Truffle] Module#deprecate_constant #4179

bjfish · 2016-09-25T21:27:35Z

I will add some questions in the diff.

bjfish · 2016-09-25T21:30:49Z

truffle/src/main/java/org/jruby/truffle/language/constants/LookupConstantNode.java

        return constant;
    }

+    @TruffleBoundary
+    private void warnDeprecatedConstant(String name) {
+        getContext().getJRubyRuntime().getWarnings().warn("constant " + name + " is deprecated");


This warning does not appear to be writing to the $stderr that the spec is expecting.

One idea I had was to use a snippet node to call warn() with a warning message. Is this an okay idea? Or is there a better way to fix this?

A SnippetNode would be nice, but it needs the frame which might be inconvenient to pass in places.

In this case a simple CallDispatchNode should be sufficient.

bjfish · 2016-09-25T21:31:21Z

...e/src/main/java/org/jruby/truffle/language/constants/LookupConstantWithLexicalScopeNode.java

+    @TruffleBoundary
+    private void warnDeprecatedConstant(String name) {
+        getContext().getJRubyRuntime().getWarnings().warn("constant " + name + " is deprecated");
+    }


This is repeated in multiple nodes. Should this be consolidated somewhere?

Yes, please make it a node.

Maybe a WarnNode that allows varargs arguments so it handle the String concatenation nicely on its side.

I've added a warn node in the latest commit

bjfish · 2016-09-25T21:32:20Z

...e/src/main/java/org/jruby/truffle/language/constants/LookupConstantWithLexicalScopeNode.java

@@ -49,6 +50,9 @@ protected RubyConstant lookupConstant(VirtualFrame frame,
        if (!isVisible) {
            throw new RaiseException(coreExceptions().nameErrorPrivateConstant(getModule(), name, this));
        }
+        if(constant != null && constant.isDeprecated()){
+            warnDeprecatedConstant(name);
+        }


Should anything be done here performance wise? A binary profile? Or something else?

No, since constant is @CompilationFinal as it is @Cached.

Interesting, there is no uncached Specialization in this node, that is a bug.

Added it in ea0c454

Please add a check also on the uncached specialization when you rebase.

I've added this check in the specialization you've added in the latest commit

eregon · 2016-09-26T10:10:29Z

truffle/src/main/java/org/jruby/truffle/core/module/ModuleFields.java

+    public void deprecateConstant(
+        final RubyContext context,
+        final Node currentNode,
+        final String name) {


Single line and no final is the current style.

I've fixed the style for this method defintion

eregon · 2016-09-26T10:13:41Z

truffle/src/main/java/org/jruby/truffle/core/module/ModuleNodes.java

+
+        @Specialization
+        public DynamicObject deprecateConstant(VirtualFrame frame, DynamicObject module, Object[] args) {
+            isFrozenNode.raiseIfFrozen(module);


There is CoreMethod.raiseIfFrozenSelf for this 😉

I've changed to use raiseIfFrozenSelf

eregon · 2016-09-26T10:15:17Z

truffle/src/main/java/org/jruby/truffle/language/constants/LookupConstantNode.java

@@ -74,9 +74,17 @@ protected RubyConstant lookupConstant(
        if (!isVisible) {
            throw new RaiseException(coreExceptions().nameErrorPrivateConstant(module, name, this));
        }
+        if(constant != null && constant.isDeprecated()){


Please try to fit in with the existing style as a general rule 😃
Maybe we should do this more automatically, like with mx eclipseformat, what do you think?

bjfish · 2016-09-27T21:11:13Z

truffle/src/main/java/org/jruby/truffle/language/constants/LookupConstantNode.java

    public Assumption getUnmodifiedAssumption(DynamicObject module) {
        return Layouts.MODULE.getFields(module).getUnmodifiedAssumption();
    }

-    @TruffleBoundary


@eregon I've removed a few TruffleBoundarys here to be able to pass the VirtualFrame parameter. Does something in these methods need to be behind a boundary? Thanks.

Yes, have a look at all calls from here.
doLookup should be boundary, or otherwise lookupConstantAndObject.
isVisible as well.
There should be a profile before the throw since isVisible is no longer compilation constant, and same goes for deprecated.

In more details: boundaries are critical since missing ones could make compilation fail.
Profiles are less but they can significantly reduce the code generated, the compilation time and improve performance (this last one not so important here since we are already in a slow path).

@eregon I've added these boundaries and profiles at 6ab74ed

eregon · 2016-09-28T09:01:46Z

...e/src/main/java/org/jruby/truffle/language/constants/LookupConstantWithLexicalScopeNode.java

@@ -45,21 +49,27 @@ public RubyConstant lookupConstant(VirtualFrame frame, Object module, String nam

    @Specialization(assumptions = "getUnmodifiedAssumption(getModule())")
    protected RubyConstant lookupConstant(
+            VirtualFrame frame,


On the line above since it's not @Cached

I've fixed this in 6ab74ed

eregon · 2016-09-28T09:02:21Z

...e/src/main/java/org/jruby/truffle/language/constants/LookupConstantWithLexicalScopeNode.java

        RubyConstant constant = doLookup();
        if (!isVisible(constant)) {
            throw new RaiseException(coreExceptions().nameErrorPrivateConstant(getModule(), name, this));
        }
+        if (constant != null && constant.isDeprecated()) {
+            warnDeprecatedConstant(frame, name);
+        }


(same applies here regarding boundaries and profiles)

I added boundaries and profiles here too.

eregon · 2016-09-28T09:03:28Z

truffle/src/main/java/org/jruby/truffle/language/WarnNode.java

+
+    @Child CallDispatchHeadNode warnMethod = CallDispatchHeadNode.createMethodCall();
+
+    public Object execute(VirtualFrame frame, Object... arguments) {


Since arguments seem to be always String for now, I would type them as such.
toString is usually a bad practice.

I've updated this to use String...

… nodes

eregon · 2016-09-29T15:09:40Z

truffle/src/main/java/org/jruby/truffle/language/WarnNode.java

-        }
-        return result;
+    private String concatArgumentsToString(String... arguments) {
+        return String.join("", arguments);


Nice Java 8 use!

eregon · 2016-09-29T15:15:42Z

truffle/src/main/java/org/jruby/truffle/language/constants/LookupConstantNode.java

-        if (!isVisible) {
+            @Cached("createBinaryProfile()") ConditionProfile sameNameProfile,
+            @Cached("createBinaryProfile()") ConditionProfile isVisibleProfile,
+            @Cached("createBinaryProfile()") ConditionProfile isDeprecatedProfile) {


No need for profiles here since constant is compilation final!

eregon · 2016-09-29T15:16:51Z

...e/src/main/java/org/jruby/truffle/language/constants/LookupConstantWithLexicalScopeNode.java

            throw new RaiseException(coreExceptions().nameErrorPrivateConstant(getModule(), name, this));
        }
+        if (isDeprecatedProfile.profile(constant != null && constant.isDeprecated())) {
+            warnDeprecatedConstant(frame, name);
+        }


same here and below regarding profiles

eregon · 2016-09-29T15:17:48Z

Getting there 😃 We just need to move the profiles to the uncached cases.

bjfish · 2016-09-29T15:58:00Z

@eregon I've moved the profiles to the uncached cases at: dc20ea8

eregon · 2016-09-29T18:46:13Z

Merged!

bjfish added the truffle label Sep 25, 2016

bjfish commented Sep 25, 2016

View reviewed changes

bjfish changed the title ~~[Truffle] module deprecate constant~~ [Truffle] Module#deprecate_constant Sep 25, 2016

eregon reviewed Sep 26, 2016

View reviewed changes

[Truffle] Start implementing Module#deprecate_constant

6141e30

bjfish force-pushed the truffle-module-deprecate-constant branch from b06f580 to c4a0217 Compare September 27, 2016 20:40

bjfish commented Sep 27, 2016

View reviewed changes

[Truffle] Add WarnNode, code formatting

95c3463

bjfish force-pushed the truffle-module-deprecate-constant branch from c4a0217 to 95c3463 Compare September 27, 2016 21:18

eregon reviewed Sep 28, 2016

View reviewed changes

[Truffle] Module#deprecate_constant add profiles/boundaries to Lookup…

6ab74ed

… nodes

eregon reviewed Sep 29, 2016

View reviewed changes

[Truffle] Module#deprecate_constant move profiles to uncached nodes

dc20ea8

eregon merged commit 439882b into truffle-head Sep 29, 2016

eregon deleted the truffle-module-deprecate-constant branch September 29, 2016 18:46

enebo modified the milestone: truffle-dev Nov 9, 2016

enebo added this to the Invalid or Duplicate milestone Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Truffle] Module#deprecate_constant #4179

[Truffle] Module#deprecate_constant #4179

bjfish commented Sep 25, 2016

bjfish Sep 25, 2016

eregon Sep 26, 2016

eregon Sep 26, 2016

bjfish Sep 25, 2016

eregon Sep 26, 2016

eregon Sep 26, 2016

bjfish Sep 27, 2016

bjfish Sep 25, 2016

eregon Sep 26, 2016

eregon Sep 26, 2016

eregon Sep 26, 2016

eregon Sep 26, 2016

bjfish Sep 27, 2016

eregon Sep 26, 2016

bjfish Sep 27, 2016

eregon Sep 26, 2016

bjfish Sep 27, 2016

eregon Sep 26, 2016

bjfish Sep 27, 2016

eregon Sep 28, 2016

eregon Sep 28, 2016

bjfish Sep 29, 2016

eregon Sep 28, 2016

bjfish Sep 29, 2016

eregon Sep 28, 2016

bjfish Sep 29, 2016

eregon Sep 28, 2016

bjfish Sep 29, 2016

eregon Sep 29, 2016

eregon Sep 29, 2016

eregon Sep 29, 2016

eregon commented Sep 29, 2016

bjfish commented Sep 29, 2016

eregon commented Sep 29, 2016


		@Child CallDispatchHeadNode warnMethod = CallDispatchHeadNode.createMethodCall();

		public Object execute(VirtualFrame frame, Object... arguments) {

[Truffle] Module#deprecate_constant #4179

[Truffle] Module#deprecate_constant #4179

Conversation

bjfish commented Sep 25, 2016

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

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

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

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

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

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

Choose a reason for hiding this comment

eregon commented Sep 29, 2016

bjfish commented Sep 29, 2016

eregon commented Sep 29, 2016