[Performance] Export Model constructor throws exceptions on very hot paths #127
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,23 +131,22 @@ public void writeTo(Object object, TreePruner pruner, DataWriter writer) throws | |
| TreePruner child = pruner.accept(object, this); | ||
| if (child==null) return; | ||
|
|
||
| Object d = writer.getExportConfig().getExportInterceptor().getValue(this,object,writer.getExportConfig()); | ||
|
|
||
|
|
||
| if ((d==null && skipNull) || d == ExportInterceptor.SKIP) { // don't write anything | ||
| return; | ||
| } | ||
| if (merge) { | ||
| // merged property will get all its properties written here | ||
| if (d != null) { | ||
| if (d.getClass().getAnnotation(ExportedBean.class) == null) { | ||
| if(writer.getExportConfig().isSkipIfFail()){ | ||
| return; | ||
| } else { | ||
| throw new NotExportableException(d.getClass()); | ||
| } | ||
| } | ||
| Model model = owner.getOrNull(d.getClass(), parent.type, name); | ||
| model.writeNestedObjectTo(d, new FilteringTreePruner(parent.HAS_PROPERTY_NAME_IN_ANCESTRY,child), writer); | ||
| } | ||
| } else { | ||
|
|
@@ -199,118 +198,126 @@ private void writeValue(Type expected, Object value, TreePruner pruner, DataWrit | |
|
|
||
| Class c = value.getClass(); | ||
|
|
||
| if (c.getAnnotation(ExportedBean.class) == null) { | ||
|
There was a problem hiding this comment. Am I missing something or would this not be caught by the null return value from There was a problem hiding this comment. You are absolutely right. Fixed. |
||
| handleNotExportable(expected, value, pruner, writer, skipIfFail, c); | ||
| } else { | ||
| try { | ||
| writer.type(expected, value.getClass()); | ||
| } catch (AbstractMethodError _) { | ||
| // legacy impl that doesn't understand it | ||
| } | ||
| try { | ||
| Model model = owner.getOrNull(c, parent.type, name); | ||
| if (model == null) { | ||
| throw new NotExportableException(c); | ||
| } | ||
| writer.startObject(); | ||
| model.writeNestedObjectTo(value, pruner, writer); | ||
| writer.endObject(); | ||
| } catch (NotExportableException ex) { | ||
| handleNotExportable(expected, value, pruner, writer, skipIfFail, c); | ||
|
There was a problem hiding this comment. reduced diff is easier to read here (generally dislike patches that change indentation of large existing code blocks but sometimes it is hard to avoid) |
||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void handleNotExportable(Type expected, Object value, TreePruner pruner, DataWriter writer, boolean skipIfFail, Class c) throws IOException { | ||
|
There was a problem hiding this comment. The method name is potentially misleading. Really this method is handling anything without There was a problem hiding this comment. Renamed and added a description. |
||
| if(STRING_TYPES.contains(c)) { | ||
| writer.value(value.toString()); | ||
| return; | ||
| } | ||
| if(PRIMITIVE_TYPES.contains(c)) { | ||
| writer.valuePrimitive(value); | ||
| return; | ||
| } | ||
| Class act = c.getComponentType(); | ||
| if (act !=null) { // array | ||
| Range r = pruner.getRange(); | ||
| writer.startArray(); | ||
| if (value instanceof Object[]) { | ||
| // typical case | ||
| for (Object item : r.apply((Object[]) value)) { | ||
| writeBuffered(act, item, pruner, writer); | ||
| } | ||
| } else { | ||
| // more generic case | ||
| int len = Math.min(r.max, Array.getLength(value)); | ||
| for (int i=r.min; i<len; i++) { | ||
| writeBuffered(act, Array.get(value, i), pruner, writer); | ||
| } | ||
| } | ||
| writer.endArray(); | ||
| return; | ||
| } | ||
| if(value instanceof Iterable) { | ||
| writer.startArray(); | ||
| Type expectedItemType = Types.getTypeArgument(expected, 0, null); | ||
| for (Object item : pruner.getRange().apply((Iterable) value)) { | ||
| writeBuffered(expectedItemType, item, pruner, writer); | ||
| } | ||
| writer.endArray(); | ||
| return; | ||
| } | ||
| if(value instanceof Map) { | ||
| if (verboseMap!=null) {// verbose form | ||
| writer.startArray(); | ||
| for (Map.Entry e : ((Map<?,?>) value).entrySet()) { | ||
| BufferedDataWriter buffer = new BufferedDataWriter(writer.getExportConfig()); | ||
| try { | ||
| writeStartObjectNullType(buffer); | ||
|
There was a problem hiding this comment. Hard to follow this long diff, but IIUC you are basically just extracting There was a problem hiding this comment. Actually the whitespace-reduced diff is more approachable; there must have been a change in indentation for a long block of code. There was a problem hiding this comment. actually, Im going to revert the method extraction. Still too long for my taste but should preserve the diff. |
||
| buffer.name(verboseMap[0]); | ||
| writeValue(null, e.getKey(), pruner, buffer); | ||
| buffer.name(verboseMap[1]); | ||
| writeValue(null, e.getValue(), pruner, buffer); | ||
| buffer.endObject(); | ||
| buffer.finished(); | ||
| } catch (IOException x) { | ||
| if (x.getCause() instanceof InvocationTargetException) { | ||
| LOGGER.log(Level.WARNING, "skipping export of " + e, x); | ||
| } | ||
| } | ||
| buffer.commit(writer); | ||
| } | ||
| writer.endArray(); | ||
| } else {// compact form | ||
| writeStartObjectNullType(writer); | ||
| for (Map.Entry e : ((Map<?,?>) value).entrySet()) { | ||
| BufferedDataWriter buffer = new BufferedDataWriter(writer.getExportConfig()); | ||
| try { | ||
| buffer.name(e.getKey().toString()); | ||
| writeValue(null, e.getValue(), pruner, buffer); | ||
| buffer.finished(); | ||
| } catch (IOException x) { | ||
| if (x.getCause() instanceof InvocationTargetException) { | ||
| LOGGER.log(Level.WARNING, "skipping export of " + e, x); | ||
| } | ||
| } | ||
| buffer.commit(writer); | ||
| } | ||
| writer.endObject(); | ||
| } | ||
| return; | ||
| } | ||
| if(value instanceof Date) { | ||
| writer.valuePrimitive(((Date) value).getTime()); | ||
| return; | ||
| } | ||
| if(value instanceof Calendar) { | ||
| writer.valuePrimitive(((Calendar) value).getTimeInMillis()); | ||
| return; | ||
| } | ||
| if(value instanceof Enum) { | ||
| writer.value(value.toString()); | ||
| return; | ||
| } | ||
|
|
||
| if (skipIfFail) { | ||
| writer.startObject(); | ||
| writer.endObject(); | ||
| return; | ||
| } | ||
|
|
||
| throw new NotExportableException(c); | ||
| } | ||
|
|
||
| private static class BufferedDataWriter implements DataWriter { | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this not redundant? AFAICT the method it delegates to does the same thing.