[Performance] Export Model constructor throws exceptions on very hot paths #127

i386 · 2017-08-16T02:59:19Z

Removes additional millis per call on large exports by avoiding throwing NotExportableException and instead using annotation check to check exportability e.g. type.getAnnotation(ExportedBean.class)

See jenkinsci/blueocean-plugin#1323

@reviewbybees #NightTimeCode

Adds additional millis per call on large exports.

ghost · 2017-08-16T03:02:13Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

vivek · 2017-08-16T18:43:02Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

+        }
+
+        try {
+            writer.type(expected, value.getClass());


This looks like 🐛 writer.type() should be called before writing the object on line 209.

vivek · 2017-08-16T22:59:15Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

-                    for (int i=r.min; i<len; i++) {
-                        writeBuffered(act, Array.get(value, i), pruner, writer);
-                    }
+            writer.type(expected, value.getClass());


It should move down inside else condition below, like 211. handleNotExportable has its own logic to write type.

right you are. Moved.

vivek

LGTM 🐝

i386 · 2017-08-17T00:31:52Z

Stapler passed locally.

i386 · 2017-08-17T00:36:12Z

@stephenc @jglick @daniel-beck a review here would be fantastic please :)

jglick

General idea sounds right. Seems to be at least one mistake here though.

jglick · 2017-08-17T00:37:23Z

core/src/main/java/org/kohsuke/stapler/export/ModelBuilder.java

@@ -42,12 +42,22 @@
    /*package*/ final Map<Class, Model> models = new ConcurrentHashMap<Class, Model>();

    public <T> Model<T> get(Class<T> type) throws NotExportableException {
+        if (type.getAnnotation(ExportedBean.class) == null) {
+            throw new NotExportableException(type);


Is this not redundant? AFAICT the method it delegates to does the same thing.

jglick · 2017-08-17T00:37:30Z

core/src/main/java/org/kohsuke/stapler/export/ModelBuilder.java

        return get(type, null, null);
    }

-    public <T> Model<T> get(Class<T> type, @CheckForNull Class<?> propertyOwner, @Nullable String property) throws NotExportableException {


Why removing the throws clause?

jglick · 2017-08-17T00:38:42Z

core/src/main/java/org/kohsuke/stapler/export/ModelBuilder.java

        return get(type, null, null);
    }

-    public <T> Model<T> get(Class<T> type, @CheckForNull Class<?> propertyOwner, @Nullable String property) throws NotExportableException {
+    public <T> Model<T> get(Class<T> type, @CheckForNull Class<?> propertyOwner, @Nullable String property) {
+        if (type.getAnnotation(ExportedBean.class) == null) {


I think you rather meant to write

Model<T> model = getOrNull(type, propertyOwner, property); if (model == null) { throw new NotExportableException(type); } return model;

jglick · 2017-08-17T00:38:49Z

core/src/main/java/org/kohsuke/stapler/export/ModelBuilder.java

        return get(type, null, null);
    }

-    public <T> Model<T> get(Class<T> type, @CheckForNull Class<?> propertyOwner, @Nullable String property) throws NotExportableException {
+    public <T> Model<T> get(Class<T> type, @CheckForNull Class<?> propertyOwner, @Nullable String property) {


@Nonnull return

jglick · 2017-08-17T00:38:57Z

core/src/main/java/org/kohsuke/stapler/export/ModelBuilder.java

+        return getOrNull(type, propertyOwner, property);
+    }
+
+    public <T> Model<T> getOrNull(Class<T> type, @CheckForNull Class<?> propertyOwner, @Nullable String property) throws NotExportableException {


@CheckForNull return. And why is this declared to throw NotExportableException?

jglick · 2017-08-17T00:39:11Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

@@ -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());
+        Object d = writer.getExportConfig().getExportInterceptor().getValue(this,object,writer.getExportConfig());


gratuitous, revert

jglick · 2017-08-17T00:42:22Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

+                model.writeNestedObjectTo(value, pruner, writer);
+                writer.endObject();
+            } catch (NotExportableException ex) {
+                handleNotExportable(expected, value, pruner, writer, skipIfFail, c);


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)

i386 · 2017-08-17T22:07:35Z

@jglick thanks for the review. There was a fair bit of dumb code here on my part, so I did my best to fix it.

jglick · 2017-08-21T23:06:35Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

+                for (Map.Entry e : ((Map<?,?>) value).entrySet()) {
+                    BufferedDataWriter buffer = new BufferedDataWriter(writer.getExportConfig());
+                    try {
+                        writeStartObjectNullType(buffer);


Hard to follow this long diff, but IIUC you are basically just extracting handleNotExportable and then calling it twice, right? Pretty much unreviewable so I hope the test coverage here is good.

Actually the whitespace-reduced diff is more approachable; there must have been a change in indentation for a long block of code.

actually, Im going to revert the method extraction. Still too long for my taste but should preserve the diff.

jglick · 2017-08-21T23:14:48Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

-            if(STRING_TYPES.contains(c)) {
-                writer.value(value.toString());
-                return;
+        if (c.getAnnotation(ExportedBean.class) == null) {


Am I missing something or would this not be caught by the null return value from getOrNull a few lines later anyway?

You are absolutely right. Fixed.

oleg-nenashev

Mostly LGTM, some minor comments

oleg-nenashev · 2017-08-22T10:23:16Z

core/src/main/java/org/kohsuke/stapler/export/ModelBuilder.java

+    }
+
+    @CheckForNull
+    public <T> Model<T> getOrNull(Class<T> type, @CheckForNull Class<?> propertyOwner, @Nullable String property) {


Needs explanation of what "null" result means (with reference to the @ExportedBean custom handling)

Nice to have: @since TODO in Javadoc

oleg-nenashev · 2017-08-22T10:27:54Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

-                        return;
-                    }
-                    throw e;
+                Model model = owner.getOrNull(d.getClass(), parent.type, name);


While you are around, it's better to move d.getClass() to a local variable. Also improves performance of code/JIT a bit

oleg-nenashev · 2017-08-22T10:28:40Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

-                    throw e;
+                Model model = owner.getOrNull(d.getClass(), parent.type, name);
+                if (model == null && !writer.getExportConfig().isSkipIfFail()) {
+                    throw new NotExportableException(d.getClass());


Maybe skipped issues deserve logging on a DEBUG level

jglick · 2017-08-22T12:53:27Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

+                for (Map.Entry e : ((Map<?,?>) value).entrySet()) {
+                    BufferedDataWriter buffer = new BufferedDataWriter(writer.getExportConfig());
+                    try {
+                        writeStartObjectNullType(buffer);


Actually the whitespace-reduced diff is more approachable; there must have been a change in indentation for a long block of code.

jglick · 2017-08-22T12:56:12Z

core/src/main/java/org/kohsuke/stapler/export/Property.java

+        }
+    }
+
+    private void handleNotExportable(Type expected, Object value, TreePruner pruner, DataWriter writer, boolean skipIfFail, Class c) throws IOException {


The method name is potentially misleading. Really this method is handling anything without @ExportedBean, but that includes all sorts of objects which are “exportable”. It is only at the very end, starting where we check skipIfFail, that this method is in fact handling objects which are not exportable.

Renamed and added a description.

jglick · 2017-09-04T01:37:52Z

core/src/main/java/org/kohsuke/stapler/export/ModelBuilder.java

@@ -56,6 +60,12 @@
        return model;
    }

+    /**
+     * Instead of throwing {@link NotExportableException} this method will return null


BTW tip: terminate summary sentences with ..

jenkinsci/stapler#127

Model constructor throws exceptions on very hot paths

d17f14c

Adds additional millis per call on large exports.

i386 changed the title ~~Model constructor throws exceptions on very hot paths~~ [Performance] Export Model constructor throws exceptions on very hot paths Aug 16, 2017

i386 mentioned this pull request Aug 16, 2017

Optimizations to REST API jenkinsci/blueocean-plugin#1323

Merged

7 tasks

Missing from downstream fork

89555a3

vivek reviewed Aug 16, 2017

View reviewed changes

Review feedback

8d3f598

vivek reviewed Aug 16, 2017

View reviewed changes

Review feedback

d76dd8c

vivek approved these changes Aug 16, 2017

View reviewed changes

jglick requested changes Aug 17, 2017

View reviewed changes

James Dumay added 2 commits August 18, 2017 07:52

Formatting change

bc2d752

Fixed my dumb code

7c2f119

jglick mentioned this pull request Aug 21, 2017

Use default method for better backward compatibility #128

Merged

jglick approved these changes Aug 21, 2017

View reviewed changes

oleg-nenashev reviewed Aug 22, 2017

View reviewed changes

jglick reviewed Aug 22, 2017

View reviewed changes

jglick mentioned this pull request Aug 22, 2017

Adding default implementations for some DataWriter methods #129

Merged

jdumay added 4 commits September 1, 2017 17:52

Review fixes

90760c3

Small updates

57dfafc

.

7821e15

merge

711abf9

jglick approved these changes Sep 4, 2017

View reviewed changes

jglick merged commit 06d268c into jenkinsci:master Sep 6, 2017

jglick added a commit that referenced this pull request Sep 6, 2017

Noting merge of #127.

d80e59a

i386 deleted the topic/performance-enhancements branch October 9, 2017 02:20

i386 pushed a commit to jenkinsci/blueocean-plugin that referenced this pull request Oct 9, 2017

Backport Stapler performance improvements

7c71f1b

jenkinsci/stapler#127

i386 mentioned this pull request Oct 9, 2017

Backport Stapler performance improvements jenkinsci/blueocean-plugin#1465

Merged

7 tasks

i386 pushed a commit to jenkinsci/blueocean-plugin that referenced this pull request Oct 9, 2017

Backport Stapler performance improvements (#1465)

4e09e9a

jenkinsci/stapler#127

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Performance] Export Model constructor throws exceptions on very hot paths #127

[Performance] Export Model constructor throws exceptions on very hot paths #127

i386 commented Aug 16, 2017 •

edited

Loading

ghost commented Aug 16, 2017

vivek Aug 16, 2017

vivek Aug 16, 2017

i386 Aug 16, 2017

vivek left a comment

i386 commented Aug 17, 2017

i386 commented Aug 17, 2017

jglick left a comment

jglick Aug 17, 2017

jglick Aug 17, 2017

jglick Aug 17, 2017

jglick Aug 17, 2017

jglick Aug 17, 2017

jglick Aug 17, 2017

jglick Aug 17, 2017

i386 commented Aug 17, 2017

jglick Aug 21, 2017

jglick Aug 22, 2017

i386 Sep 2, 2017

jglick Aug 21, 2017

i386 Sep 2, 2017

oleg-nenashev left a comment

oleg-nenashev Aug 22, 2017

i386 Sep 2, 2017

oleg-nenashev Aug 22, 2017

i386 Sep 2, 2017

oleg-nenashev Aug 22, 2017

jglick Aug 22, 2017

jglick Aug 22, 2017

i386 Sep 2, 2017

jglick Sep 4, 2017

[Performance] Export Model constructor throws exceptions on very hot paths #127

[Performance] Export Model constructor throws exceptions on very hot paths #127

Conversation

i386 commented Aug 16, 2017 • edited Loading

ghost commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivek left a comment

Choose a reason for hiding this comment

i386 commented Aug 17, 2017

i386 commented Aug 17, 2017

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i386 commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i386 commented Aug 16, 2017 •

edited

Loading