-
Notifications
You must be signed in to change notification settings - Fork 11
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
Custom Backtrace serializer/deserializer - GSON replacement #162
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # backtrace-library/build.gradle # backtrace-library/src/main/java/backtraceio/library/models/json/BacktraceAttributes.java
# Conflicts: # backtrace-library/src/main/java/backtraceio/library/models/json/BacktraceReport.java
// GSON | ||
long startTime = System.currentTimeMillis(); | ||
BacktraceSerializeHelper.toJson(data); | ||
long endTime = System.currentTimeMillis(); | ||
timeGson += endTime - startTime; | ||
|
||
// ORG JSON | ||
long startTimeOrg = System.currentTimeMillis(); | ||
BacktraceDataSerializer serializer = new BacktraceDataSerializer(new NamingPolicy()); | ||
serializer.toJson(data); | ||
long endTimeOrg = System.currentTimeMillis(); | ||
timeOrgJson += endTimeOrg - startTimeOrg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be nice to test one-time serialization also?
I'm thinking about the FieldNameCache initialization, which may take some more time on the first go.
if (obj == null) { | ||
return null; | ||
} | ||
SourceCodeDeserializer deserializer = new SourceCodeDeserializer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit: maybe keep this (and every other serializer) as class fields, so you won't have to create them each time?
try { | ||
if (obj.get(key) instanceof JSONObject) { | ||
result.put(key, deserializer.deserialize((JSONObject) obj.get(key))); | ||
} | ||
} catch (JSONException e) { | ||
BacktraceLogger.e(LOG_TAG, String.format("Exception on deserialization of source code, " + | ||
"key %s, object %s", key, obj), e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we try/catch the whole object serialization? Returning a "valid" deserialized object with it's data missing is a code smell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This applies for every try/catch in this class and other deserializers.
final ThreadInformationDeserializer deserializer = new ThreadInformationDeserializer(); | ||
Iterator<String> keys = obj.keys(); | ||
|
||
while (keys.hasNext()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you guys have foreach
? 😢
if (jsonArray == null) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this codebase expect ever BacktraceData.classifiers
to be null? Maybe return an empty array instead?
|
||
JSONArray jsonArray = new JSONArray(); | ||
for (Object item : array) { | ||
jsonArray.put(serialize(namingPolicy, item, serializationDepth)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsonArray.put(serialize(namingPolicy, item, serializationDepth)); | |
jsonArray.put(serialize(namingPolicy, item, serializationDepth + 1)); |
|
||
JSONArray jsonArray = new JSONArray(); | ||
for (Object item : collection) { | ||
jsonArray.put(serialize(namingPolicy, item, serializationDepth)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsonArray.put(serialize(namingPolicy, item, serializationDepth)); | |
jsonArray.put(serialize(namingPolicy, item, serializationDepth + 1)); |
} | ||
|
||
public static boolean isPrimitiveType(Object source) { | ||
return WRAPPER_TYPE_MAP.containsKey(source.getClass()) || source instanceof String || source instanceof Number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't you also check for non-wrapped types, such as int.class
?
|
||
private static Object serializeException(NamingPolicy namingPolicy, Exception exception) { | ||
try { | ||
return getAllFields(namingPolicy, exception.getClass(), exception, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 2?
for (Map.Entry<?, ?> entry : map.entrySet()) { | ||
String key = String.valueOf(entry.getKey()); | ||
Object value = entry.getValue(); | ||
jsonObject.put(key, serialize(namingPolicy, value, serializationDepth)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jsonObject.put(key, serialize(namingPolicy, value, serializationDepth)); | |
jsonObject.put(key, serialize(namingPolicy, value, serializationDepth + 1)); |
Important note: First we need to merge PR #144
Summary
This PR contains all changes needed to replace GSON library which was used for serialization/deserialization data. These modifications are part of the ongoing efforts to optimize the backtrace-android library by reducing external dependencies and enhancing performance.
Key Changes
Performance comparison GSON vs custom solution based on included unit-tests
BacktraceData serialization (SerializerBacktraceDataGsonComparisonTest.java)
Different objects serialization (SerializerGsonComparisonTest.java)
Different object deserialization (DeserializerGsonComparisonTest.java)
Potential breaking changes
import com.google.gson.annotations.SerializedName;
you need to replace it withimport backtraceio.library.common.json.serialization.SerializedName;
and verify if your objects are serialized/deserialized correctly before going to production.Architecture of new serialization/deserialization flow

