Dismantle instanceof checks #16

Open
frankiesardo opened this Issue Feb 15, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@frankiesardo
Owner

frankiesardo commented Feb 15, 2015

As noted in this benchmark writeValue slows down Parcelable serialisation. Using the type of the field to emit the appropriate method call should increase the speed of ~ 2X and would be a tactical move to later tackle #7

@PaulWoitaschek

This comment has been minimized.

Show comment
Hide comment
@PaulWoitaschek

PaulWoitaschek Oct 18, 2015

Why would speed that creation? I just checked the source and the current implementation looks like this:

    public final void writeValue(Object v) {
        if (v == null) {
            writeInt(VAL_NULL);
        } else if (v instanceof String) {
            writeInt(VAL_STRING);
            writeString((String) v);
        } else if (v instanceof Integer) {
            writeInt(VAL_INTEGER);
            writeInt((Integer) v);
        } else if (v instanceof Map) {
            writeInt(VAL_MAP);
            writeMap((Map) v);
        } else if (v instanceof Bundle) {
            // Must be before Parcelable
            writeInt(VAL_BUNDLE);
            writeBundle((Bundle) v);
        ...

so the part for serializable is

                writeInt(VAL_SERIALIZABLE);
                writeSerializable((Serializable) v);

What would be optimized here?

Another thought I had is about enums. If I get it correctly the current implementation passes enums by passing it as Serializable.
But wouldn't as enums are per definition singletons - the approach to pass them by String name = enum.name() and restore them by EnumType.valueOf(name) be a way more efficient way?

Why would speed that creation? I just checked the source and the current implementation looks like this:

    public final void writeValue(Object v) {
        if (v == null) {
            writeInt(VAL_NULL);
        } else if (v instanceof String) {
            writeInt(VAL_STRING);
            writeString((String) v);
        } else if (v instanceof Integer) {
            writeInt(VAL_INTEGER);
            writeInt((Integer) v);
        } else if (v instanceof Map) {
            writeInt(VAL_MAP);
            writeMap((Map) v);
        } else if (v instanceof Bundle) {
            // Must be before Parcelable
            writeInt(VAL_BUNDLE);
            writeBundle((Bundle) v);
        ...

so the part for serializable is

                writeInt(VAL_SERIALIZABLE);
                writeSerializable((Serializable) v);

What would be optimized here?

Another thought I had is about enums. If I get it correctly the current implementation passes enums by passing it as Serializable.
But wouldn't as enums are per definition singletons - the approach to pass them by String name = enum.name() and restore them by EnumType.valueOf(name) be a way more efficient way?

@frankiesardo

This comment has been minimized.

Show comment
Hide comment
@frankiesardo

frankiesardo Oct 19, 2015

Owner

The optimisation is to avoid emitting writeValue and use instead writeString, writeInt etc. instanceof checks are quite expensive it seems

Owner

frankiesardo commented Oct 19, 2015

The optimisation is to avoid emitting writeValue and use instead writeString, writeInt etc. instanceof checks are quite expensive it seems

@PaulWoitaschek

This comment has been minimized.

Show comment
Hide comment
@PaulWoitaschek

PaulWoitaschek Nov 3, 2015

I tried to dig into this a little bit. I don't know too much about velocity but this is a first approach (which works) I came up with.

I don't really get how it produces the whitespace etc but this one works.
What it does is it checks the types and starts replacing String and long with their representation. This way many more types could be replaced.

So this is what it produces:

  @Override
  public void writeToParcel(android.os.Parcel dest, int flags) {
    dest.writeString(name);
    dest.writeLong(id);
    dest.writeValue(heightType);
    dest.writeValue(addresses);
    dest.writeValue(friends);
  }

  private AutoParcel_Person(android.os.Parcel in) {
    this(in.readString(), in.readLong(), (HeightBucket) in.readValue(CL), (Map<String, Address>) in.readValue(CL), (List<Person>) in.readValue(CL));
  }

Do you think this is a wise approach to follow? How to I get the

Here comes the autoparcel.vm

  @Override
  public void writeToParcel(android.os.Parcel dest, int flags) {
#foreach ($p in $props)
  #set ($type = $p.getType())##
  #if($type=="String")##
  dest.writeString($p)##
  #elseif($type=="long")##
  dest.writeLong($p)##
  #else##
  dest.writeValue($p)#end;
#end
  }

  private $subclass(android.os.Parcel in) {
    this(#foreach ($p in $props)
    #set ($type = $p.getType())
    #if($type=="String")in.readString()##
    #elseif($type=="long")in.readLong()##
    #else
      ($p.castType) in.readValue(CL)##
    #end
    #if ($foreach.hasNext),##
    #end
    #end);
  }

I tried to dig into this a little bit. I don't know too much about velocity but this is a first approach (which works) I came up with.

I don't really get how it produces the whitespace etc but this one works.
What it does is it checks the types and starts replacing String and long with their representation. This way many more types could be replaced.

So this is what it produces:

  @Override
  public void writeToParcel(android.os.Parcel dest, int flags) {
    dest.writeString(name);
    dest.writeLong(id);
    dest.writeValue(heightType);
    dest.writeValue(addresses);
    dest.writeValue(friends);
  }

  private AutoParcel_Person(android.os.Parcel in) {
    this(in.readString(), in.readLong(), (HeightBucket) in.readValue(CL), (Map<String, Address>) in.readValue(CL), (List<Person>) in.readValue(CL));
  }

Do you think this is a wise approach to follow? How to I get the

Here comes the autoparcel.vm

  @Override
  public void writeToParcel(android.os.Parcel dest, int flags) {
#foreach ($p in $props)
  #set ($type = $p.getType())##
  #if($type=="String")##
  dest.writeString($p)##
  #elseif($type=="long")##
  dest.writeLong($p)##
  #else##
  dest.writeValue($p)#end;
#end
  }

  private $subclass(android.os.Parcel in) {
    this(#foreach ($p in $props)
    #set ($type = $p.getType())
    #if($type=="String")in.readString()##
    #elseif($type=="long")in.readLong()##
    #else
      ($p.castType) in.readValue(CL)##
    #end
    #if ($foreach.hasNext),##
    #end
    #end);
  }
@frankiesardo

This comment has been minimized.

Show comment
Hide comment
@frankiesardo

frankiesardo Nov 4, 2015

Owner

While I appreciate your effort, the library will undergo a major refactoring soon to catch up with the latest AutoValue extensions. I'm unlikely to merge anything before that and most things will be unmergeable after that. I'll resume this work after 1.0 release.

Owner

frankiesardo commented Nov 4, 2015

While I appreciate your effort, the library will undergo a major refactoring soon to catch up with the latest AutoValue extensions. I'm unlikely to merge anything before that and most things will be unmergeable after that. I'll resume this work after 1.0 release.

@PaulWoitaschek

This comment has been minimized.

Show comment
Hide comment
@PaulWoitaschek

PaulWoitaschek Nov 9, 2015

Yeah no problem. I mostly wanted to learn something new.

And I did only parcel the 2 types to see what you are thinking. But it proves that this works so later something like this can be integrated.

Yeah no problem. I mostly wanted to learn something new.

And I did only parcel the 2 types to see what you are thinking. But it proves that this works so later something like this can be integrated.

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