Skip to content
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

[Truffle] OM DSL #3172

Closed
wants to merge 3 commits into from
Closed

[Truffle] OM DSL #3172

wants to merge 3 commits into from

Conversation

chrisseaton
Copy link
Contributor

This is the next piece in the puzzle for the transition to pure DynamicObject and OM. Instead of specifying properties manually for all our existing core classes, this DSL generates them for you from an interface which makes it look a bit more like there's a real class there.

@nirvdrum @pitr-ch @eregon @woess @thomaswue please review.

As you can't see the generated code in the PR, it converts this:

@Layout
public interface ArrayLayout {

    DynamicObject createArray(Object store, int size);

    boolean isArray(DynamicObject object);

    Object getStore(DynamicObject object);

    void setStore(DynamicObject object, Object store);

    int getSize(DynamicObject object);

    void setSize(DynamicObject object, int size);

}

To this:

package org.jruby.truffle.nodes.core.array;

import java.util.EnumSet;
import com.oracle.truffle.api.object.*;
import org.jruby.truffle.om.dsl.api.UnexpectedLayoutRefusalException;
import org.jruby.truffle.nodes.core.array.ArrayNodes.ArrayLayout;

public class ArrayLayoutImpl implements ArrayLayout {

    public static final ArrayLayout INSTANCE = new ArrayLayoutImpl();

    private static class ArrayType extends ObjectType {

    }

    private final ArrayType ARRAY_TYPE = new ArrayType();

    private final HiddenKey STORE_IDENTIFIER = new HiddenKey("store");
    private final Property STORE_PROPERTY;

    private final HiddenKey SIZE_IDENTIFIER = new HiddenKey("size");
    private final Property SIZE_PROPERTY;

    private final DynamicObjectFactory ARRAY_FACTORY;

    private ArrayLayoutImpl() {
        final Layout layout = Layout.createLayout(Layout.INT_TO_LONG);
        final Shape.Allocator allocator = layout.createAllocator();

        STORE_PROPERTY = Property.create(STORE_IDENTIFIER, allocator.locationForType(java.lang.Object.class, EnumSet.of(LocationModifier.NonNull)), 0);
        SIZE_PROPERTY = Property.create(SIZE_IDENTIFIER, allocator.locationForType(int.class, EnumSet.of(LocationModifier.NonNull)), 0);

        final Shape shape = layout.createShape(ARRAY_TYPE)
            .addProperty(STORE_PROPERTY)
            .addProperty(SIZE_PROPERTY);

        ARRAY_FACTORY = shape.createFactory();
    }

    @Override
    public DynamicObject createArray(java.lang.Object store,
            int size) {
        return ARRAY_FACTORY.newInstance(store,
            size);
    }

    @Override
    public boolean isArray(DynamicObject object) {
        return object.getShape().getObjectType() == ARRAY_TYPE;
    }

    @Override
    public java.lang.Object getStore(DynamicObject object) {
        assert isArray(object);
        assert object.getShape().hasProperty(STORE_IDENTIFIER);

        return (java.lang.Object) STORE_PROPERTY.get(object, true);
    }

    @Override
    public void setStore(DynamicObject object, java.lang.Object value) {
        assert isArray(object);
        assert object.getShape().hasProperty(STORE_IDENTIFIER);

        try {
            STORE_PROPERTY.set(object, value, object.getShape());
        } catch (IncompatibleLocationException | FinalLocationException e) {
            throw new UnexpectedLayoutRefusalException(e);
        }
    }

    @Override
    public int getSize(DynamicObject object) {
        assert isArray(object);
        assert object.getShape().hasProperty(SIZE_IDENTIFIER);

        return (int) SIZE_PROPERTY.get(object, true);
    }

    @Override
    public void setSize(DynamicObject object, int value) {
        assert isArray(object);
        assert object.getShape().hasProperty(SIZE_IDENTIFIER);

        try {
            SIZE_PROPERTY.set(object, value, object.getShape());
        } catch (IncompatibleLocationException | FinalLocationException e) {
            throw new UnexpectedLayoutRefusalException(e);
        }
    }

}

@chrisseaton chrisseaton added this to the truffle-dev milestone Jul 23, 2015
@chrisseaton chrisseaton self-assigned this Jul 23, 2015
@nirvdrum
Copy link
Contributor

I need to give the code a deeper review, but is there a mechanism to handle default values when a property is missing? E.g., we do this to avoid explicitly saving frozen or taint state for certain object types. Or is an override just the preferred approach?

@chrisseaton
Copy link
Contributor Author

The frozen and taint fields aren't always in the layout so don't have a static location and therefore they need a node in order to specialise. We could look at generating those nodes as well - but we only have a couple of them.

Other features I will add when I get to classes that need them: nullable and non-nullable properties, final properties, factory methods with default values maybe, configuration such as the int-to-long thing, inheritance, storing some fields in the shape, not the object, such as the Ruby class.

@eregon
Copy link
Member

eregon commented Jul 24, 2015

Neat!
For reading/writing in the Shape, we will likely need nodes as currently to do that we need to do a Shape check.

@pitr-ch
Copy link
Member

pitr-ch commented Jul 24, 2015

Nice!

* }
* </pre>
*
* The properties are defined by getters and setter method pairs. They should
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issue, but I'd think "getters" should be plural here if used as a component of the pair description.

@nirvdrum
Copy link
Contributor

Looks great. I think the docs in Layout are extremely helpful. However, it would also be nice to have brief class-level docs for each of the new classes -- even if they just link back to the Layout docs.

@jtulach
Copy link
Contributor

jtulach commented Jul 30, 2015

I assume one often has multiple ArrayLayout-like interfaces in a package. Then I'd suggest to generate one file for the whole package named Layouts(?) with public static final ARRAY_LAYOUT and private class ArrayLayoutImpl. It would lower "conceptual surface" of the generated API by make impl classes invisible.

@chrisseaton
Copy link
Contributor Author

Good idea, but I'm not sure it's possible to guarantee that your annotation processor will see all files in a single pass and so be able to do that. For example in incremental compilation in Eclipse, you won't see files which haven't changed.

http://stackoverflow.com/questions/13397987/annotationprocessor-using-multiple-source-files-to-create-one-file

@jtulach
Copy link
Contributor

jtulach commented Aug 2, 2015

I expanded the stackoverflow answer a bit. It should be possible to get access to all elements in a package even in case of incremental compilation.

@chrisseaton
Copy link
Contributor Author

Ah thanks for that - I'll take a look when I've got the next part of the DSL working.

@enebo enebo added this to the Non-Release milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants