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

Definition cleanup #18463

Merged
merged 24 commits into from May 20, 2016

Conversation

Projects
None yet
4 participants
@rmuir
Copy link
Contributor

commented May 19, 2016

Painless creates its own class hierarchy backed by java classes and methods. But this is all in code and has become cumbersome (i dont want to add more methods in the current state because of it).

So, it would be good to move this definition to a file, where its easier to maintain. Also all the information about a class should be together.

This moves it out to a file with a simple format, easy to parse with low-tech, just defines class and superclasses and then signatures.

class Integer -> java.lang.Integer extends Number,Object {
  Integer <init>(int)
  int MIN_VALUE
  int MAX_VALUE
  int compare(int,int)
  int compareTo(Integer)
  String toHexString(int)
  ...
}

Other problems were hardcoding every class inside Definition and then referring to those elsewhere. I changed all these to be a proper lookup (Definition.getType). We have to start making impl details impl details for Definition and instead define operations like this. This change caused a lot of noise, but we need to start separating the stuff. i think in the future if we cleanup Definition, we could e.g. define static final constants for the primitive types and it would look nicer (this requires more refactoring).

primitive types are in our definition file, which may look strange, but we may want to define methods for them. transforms are still hardcoded but those need separate cleanups and I think a lot can be removed. the "ghetto generics" are removed.

The difficulty with cleaning up this (and why i did a half-ass job) is that it takes many hours of slave labor to make a single change. That is why we have to start cleaning it up, it gets easier as we go through it. But its still kinda painful. I just stopped from exhaustion but not because I consider it done or even in a great state, just better.

@jdconrad

This comment has been minimized.

Copy link
Contributor

commented May 19, 2016

LGTM! Awesome, thank you for making this change!

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 19, 2016

I will more closely look into this later. Looks good as a first step!

@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2016

thanks @uschindler . There are many more steps. I don't care how we do it, if you want just push improvements to my branch or we can make more PRs.

I do think we should avoid cleaning up the grammar, just to be practical. @jdconrad is working heavily on this now and i dont want to conflict with that. From what I see, the generics syntax does not cause any of the problems the grammar has today but it will still simplify to get rid of it later.

rmuir added some commits May 19, 2016

dce
@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 19, 2016

Hi, thanks for the invitation to colaborate in this PR. I like the first step, but I am not happy with removing the "constants" and replacing them by getters with strings ("def", "int",...). I know why you did this, so I would take care of changing this:

Remove the copy constructor. I would make the inner part that holds the maps completely private and allocate one singleton instance using a completely private ctor (the instance field would be static final and also private). The maps inside do not necessarily be unmodifiable (we can do this later), but instead of the copy constructor, we should add static getters and static final constants that delegate to the private singleton instance, which is completely private. For consumers of definition clas,s they only see static final constants and some static, read-only getter methods. I will think about that this night and start to implement this tomorrow.

@@ -72,14 +72,24 @@

private static final MethodHandle OBJECT_ARRAY_MH = ARRAY_TYPE_MH_MAPPING.get(Object[].class);

// NOTE: the following are actually used, javac just does not know :)
@SuppressWarnings("unused")

This comment has been minimized.

Copy link
@uschindler

uschindler May 19, 2016

Contributor

I have seen this, too (in Eclipse only). I would like to add the SuppressWarnings to the whole inner class holding that methods, not every method. I did not do that because I have seen it too late after the PR was already committed.
BTW: The PR for Java 9 for adding the missing getter was approved and was extended by a compiler instrinsic! I will think about using the Java 9 method to get the array length getter with a separate MH to get the arraylength MH.

This comment has been minimized.

Copy link
@jdconrad

jdconrad May 19, 2016

Contributor

@uschindler That's awesome about the missing getters. Thanks for making that happen!

@jdconrad jdconrad referenced this pull request May 19, 2016

Closed

Ongoing Painless Improvements #17992

15 of 18 tasks complete

jdconrad added some commits May 20, 2016

uschindler and others added some commits May 20, 2016

Make Definition's public API completely static.
TODO: Remove Definition arguments everywhere and hide INSTANCE field!
@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

I made the public API of definition completely static. The INSTANCE field is temporarily still there, because I have to first remove the Definition parameter passed everywhere.

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

OK, I remved the definition parameter on all methods around analyze and write.

uschindler and others added some commits May 20, 2016

Remove Definitions's copy-ctor; fix RuntimeClass to be unmodifiable
Please note: The maps inside the pirvate singleton instance of Defininition are no longer unmodifiable, but nothing from the outside can modify it! All private :-)
@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

Thanks @uschindler for the cleanups here!!!! I pushed a couple minor cleanups, now that those internal-used types are constants, they are capitalized. And we don't need to pass CompilerSettings to every method in tree nodes.

I think this is good for now? I can think of quite a few followup cleanups but lets just get these simplifications in.

Thanks again for helping with cleanups!

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

Hi @rmuir, all fine from my side. Thanks for also cleaning up the compiler settings. This was the next on my TODO list. :-)

I think you should merge that now. If we do further improvements, those should be more focused on certain classes.

@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

@jdconrad pinged me last night as he did his cleanup to boxed types in another branch. I pulled it in as its related: now there is no more transforms map, no more confusing Def vs def at all, no Utility stuff in whitelist, much better!

I agree we should stop now and get some of this in :)

@jdconrad

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

@rmuir and @uschindler Thanks for all the help with cleaning this stuff up guys. I know it can be massively time consuming.

@jdconrad

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

@uschindler An ASM question -- I noticed that for int -> Integer boxing the javac compiler will issue the instruction for Integer.valueOf, but using the GeneratorAdapter to do boxing, the instructions issued use new Integer(...), dup_x1, swap. Is this anything to worry about, should we be using Integer.valueOf here instead?

Edit: @rmuir has pointed out that I meant valueOf instead of parseInt. Oops!

@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

+1 to fix that, that is wrong to do (i dont care about java 1.4 here).

Can we override that method in our adapter? http://websvn.ow2.org/filedetails.php?repname=asm&path=%2Ftrunk%2Fasm%2Fsrc%2Forg%2Fobjectweb%2Fasm%2Fcommons%2FGeneratorAdapter.java

@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

I would just override this one for now: public void box(final Type type) {. We can do the right thing. Their unbox is fine.

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

We should not trust everything ASM is doing...

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

I would just override this one for now: public void box(final Type type) {. We can do the right thing. Their unbox is fine.

+1 (use the code from the svn link)

    /**
     * Generates the instructions to box the top stack value using Java 5's
     * valueOf() method. This value is replaced by its boxed equivalent on top
     * of the stack.
     * 
     * @param type
     *            the type of the top stack value.
     */
    public void valueOf(final Type type) {
        if (type.getSort() == Type.OBJECT || type.getSort() == Type.ARRAY) {
            return;
        }
        if (type == Type.VOID_TYPE) {
            push((String) null);
        } else {
            Type boxed = getBoxedType(type);
            invokeStatic(boxed, new Method("valueOf", boxed,
                    new Type[] { type }));
        }
    }
``
@jdconrad

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

@uschindler Thanks for the pointing that out. I've updated the branch to use valueOf instead of box.

@Override
public void box(final org.objectweb.asm.Type type) {
switch (type.getSort()) {
case org.objectweb.asm.Type.VOID: visitInsn(Opcodes.ACONST_NULL); break;

This comment has been minimized.

Copy link
@uschindler

uschindler May 20, 2016

Contributor

I'd remove this again. Because this is done in original ASM, just to prevent incorrect stack on voids

@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

Can we override box to just call valueOf though? I dont like the method being there with inefficient code, waiting for something to call it.

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

Can we override box to just call valueOf though?

+1 to remove the trap. Then code can call box() or valueOf()

@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

ok i am happy, trap is gone. thanks @jdconrad for discovering this trap

@uschindler

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

Thanks!

@rmuir rmuir merged commit a2ff002 into elastic:master May 20, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details
@rmuir

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2016

thanks guys for all the cleanup help! It is worth it here...

@clintongormley clintongormley changed the title painless: steps at definition cleanup Definition cleanup May 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.