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

* added forceLinkMethods configuration #322

Merged
merged 1 commit into from Sep 18, 2018

Conversation

Projects
None yet
5 participants
@dkimitsa
Contributor

dkimitsa commented Aug 27, 2018

this parameter and handling forces methods to be kept from removing by aggressive tree shaker. Here is an example of issue:

private void testSerialize() {
        try {
            new ObjectOutputStream(new ByteArrayOutputStream()).writeObject(new HashMap<>());
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

throws:

java.io.InvalidClassException: java.util.HashMap doesn't have a field loadFactor of type float
	at java.io.ObjectOutputStream.writeFieldValues(ObjectOutputStream.java:955)
* added forceLinkMethods configuration parameter and handling to keep…
… not referenced methods from getting removed
@dkimitsa

This comment has been minimized.

Contributor

dkimitsa commented Aug 27, 2018

details in the post: https://dkimitsa.github.io/2018/08/27/aggressive-treeshaker-workarounds/
Please let me know if this can be done other way I missed.

@dkimitsa

This comment has been minimized.

Contributor

dkimitsa commented Aug 28, 2018

methods to be forced linked has to be configured using forceLinkMethods tag in robovm.xml similar to forceLinkClasses. It has following format:

  <forceLinkMethods>
        <entry>
            <methods>
                <method>writeObject(Ljava/io/ObjectOutputStream;)V</method>
                <method>readObject(Ljava/io/ObjectInputStream;)V</method>
            </methods>
            <owners>
                <pattern extendable="true">java.io.Serializable</pattern>>
            </owners>
        </entry>
    </forceLinkMethods>

Under <forceLinkMethods> can go one or more <entry> tag. Each <entry> specifies class filtering criteria and method signatures to be forced linked.

<owners> tag specifies one or more patterns class to be matched for force linking methods. if
extendable="true" attribute is specified class is checked to match pattern as well as its superclasses are matched and all interfaces it (and superclasses) implements are matched.

<methods> specifies one or more methods to be force linked in matched classes. Format of entry is concatenation of method name with its full JVM signature. In example above:

writeObject(Ljava/io/ObjectOutputStream;)V

corresponds to

void writeObject(ObjectOutputStream stream)

here is an snippet from java doc that explains how types are mapped to signatures:

Type Signature Java Type
Z boolean
B byte
C char
S short
I int
J long
F float
D double
L fully-qualified-class ; fully-qualified-class
[ type type[]
( arg-types ) ret-type method type

For example, the Java method:

long f (int n, String s, int[] arr);

has the following type signature:

(ILjava/lang/String;[I)J

@keesvandieren

This comment has been minimized.

Contributor

keesvandieren commented Aug 30, 2018

As this is to be included in release 2.3.5, someone has to review it. I have no clue about this, maybe @achabdo can have a look?

@achabdo

This comment has been minimized.

achabdo commented Aug 30, 2018

I will take a look @keesvandieren,but I guess that @dkimitsa is trying to kept methods used for serialization declared in classes which implement the java.io.Serializable interface from been removed by the treeshaker as they are used for reflection ,I will suggest a better solution which avoid user expliciting those methods.
You declare a method which verifie if a class implement the java.io.Serializable interface:
private static boolean isSerializable(SootClass clazz){
while(clazz != null){
if(clazz.getInterfaces().contains(Scene.v().getSootClass("java.io.Serializable")))
return true;
clazz = clazz.getSuperclass();
}
return false;
}

later you can use this in the DependecyGraph.java and you verfie if a class implement the Serializable interface ,if this is the case then all the methods from the class are considered to be "root"

boolean strong = root
// Keep callback methods
|| mi.isCallback()
// Keep class initializers
|| (mi.isStatic() && "".equals(mi.getName()) && "()V".equals(mi.getDesc()))
// Keep the values() method generated by the Java compiler
// in enum classes
|| (ci.isEnum() && mi.isStatic() && "values".equals(mi.getName()) && mi.getDesc().equals(
"()[L" + clazz.getInternalName() + ";"))
// Keep the sizeOf() method generated by the RoboVM compiler
// in Struct classes
|| (ci.isStruct() && mi.isStatic() && "sizeOf".equals(mi.getName()) && "()I".equals(mi.getDesc()));
|| isSerializable ( ci.getClazz().getSootClass()); //add this line

but I think you can use this future for another purpose like avoiding including all the methods and fields of class with -forcelinkclasses and use -forcelinkmethod to choose specific methods only to reduce application size.

@dkimitsa

This comment has been minimized.

Contributor

dkimitsa commented Aug 30, 2018

@keesvandieren this feature will not affect generic case when this key is not included. I've tested but as developer I'm under suspect part and need confirmation from third party that nothing is broken

@achabdo sorry but you are wrong. It is not about java.io.Serializable but any class we need to protect methods in from tree shaker. Serializable was demonstrated in configuration as bright example but there can be put any foo class. And you can't use this feature for reducing size. Actually it is for increasing size. As it will keep methods that aggressive tree shaker is usually removes.

@achabdo

This comment has been minimized.

achabdo commented Aug 30, 2018

I don't understood ?we have already the forcelinkclass option to avoid the treeshaker from stripping methods? no?I guess the purpose is including specific methods rather including a whole class ?So it's about reducing the size @dkimitsa ?

@dkimitsa

This comment has been minimized.

Contributor

dkimitsa commented Aug 30, 2018

forceLinkClasses is used to explicitly specify classes to be compiled even if they are no not explicitly referenced.

The case with aggressive tree shaker is that it removes method that are not referenced from classes that are explicitly referenced. For example whenHashMap is explicitly referenced there is a bunch of methods will be dropped if they are not explicitly referenced by app.
But these methods might be required runtime and accessible using reflection.

forceLinkMethods is designed to keep these methods from being dropped. Of course we can list all classes with forceLinkClasses to be put into root classes list. but it will require you to know exact list to be included which is not alway possible (for ex. all Serializable ones). And also it will include all methods of these classes and not only one we are interested in.

@achabdo

This comment has been minimized.

achabdo commented Aug 30, 2018

this is looking good.It should be included in the 2.3.5 release.

@intrigus

This comment has been minimized.

Contributor

intrigus commented Aug 30, 2018

Is anybody even using aggressive three shaking?
If we add forceLinkMethods wouldn't we also have to add forceLinkField to force link class fields?

I still think, that the special handling of j.io.Serializable should not be put in a configuration file that can be changed by the user.

@dkimitsa

This comment has been minimized.

Contributor

dkimitsa commented Aug 31, 2018

@intrigus

  • aggressive tree shaking is default for framework target
  • fields are not subject for three shaker
  • serializable here is for demonstration purpose only, changes are for generic case. it to be put in configuration, and it is used by user. Configuration wise its meaning pretty same as forceLinkClasses
@achabdo

This comment has been minimized.

achabdo commented Aug 31, 2018

Fields can be subject to tree shaking @dkimitsa ,I tried this in the past but the benefit will not be such as big,you can find reachable fields from reachableMethods .
for serialization I will go with @intrigus it should be handled by the compiler automaticly because most java programs use it.for other cases wich are application dependent user may have to explicit those methods with -forceLinkMethods like you suggested before.

@dkimitsa

This comment has been minimized.

Contributor

dkimitsa commented Aug 31, 2018

@achabdo

  • please don't mislead people. there is no filed tree shaking in today master
  • I believe there is much more cases around reflection beside serializable (which I shown here just as example). instead of hardcoding every case in compiler I gave a tool for this. As most people doesn't use aggressive mode -- they are not affected. Who uses -- they know what they are doing and just need a tool (that was missing before)

anyway please stop leading this into academical and demagogy way and keep practical side. Today we have a problem and need solution to be applied.

@achabdo

This comment has been minimized.

achabdo commented Aug 31, 2018

@dkimitsa

  • I did not mention that there was a field tree shaking in master today, I said you could do one, and it would be useless in many cases because you would only remove a few kilobytes in response to our friend @intrigus .
  • "Stop leading this to academia and demagoguery" no comment.

@florianf florianf merged commit 953a4e6 into MobiVM:master Sep 18, 2018

@florianf

This comment has been minimized.

Collaborator

florianf commented Sep 18, 2018

Thx. There are valid use cases for this.

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