Optimizations to REST API #1323

i386 · 2017-08-14T06:57:42Z

Description

Serving this request took 75ms. It now takes 30ms.

Submitter checklist

Link to JIRA ticket in description, if appropriate.
Change is code complete and matches issue description
Appropriate unit or acceptance tests or explanation to why this change has no tests
Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

Run the changes and verified the change matches the issue description
Reviewed the code
Verified that the appropriate tests have been written or valid explanation given

…lliseconds to export

It will be cached and the action lookup here wont happen

i386 · 2017-08-14T07:31:19Z

@vivek I am not sure how to interpret the failure here - is it just saying I need more tests?

vivek · 2017-08-14T18:31:27Z

@i386 look in ${project.build.directory}/code-coverage/report, there should be index.html, that should give you details.

vivek · 2017-08-14T18:33:07Z

blueocean-commons/src/main/java/io/jenkins/blueocean/commons/stapler/export/ModelBuilder.java

@@ -44,12 +44,26 @@
    /*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) {


Directly updating stapler imported code? This can leave us with pain later on, better to submit PR on stapler then we import them over here.

Done jenkinsci/stapler#127

vivek · 2017-08-14T18:34:12Z

blueocean-commons/src/main/java/io/jenkins/blueocean/commons/stapler/export/Property.java

-                try {
-                    model = owner.get(d.getClass(), parent.type, name);
-                } catch (NotExportableException e) {
+                if (d.getClass().getAnnotation(ExportedBean.class) == null) {


Same as above, lets not update stapler code directly here. Please open a PR on stapler.

Changes in Property.java in this PR is different from the ones in PR. I think its cleaner to remove changes from here and merge stapler changes in separate PR, after stapler PR is merged. This way we can merge this PR without waiting for stapler changes and get immediate perf benefits.

vivek · 2017-08-14T18:43:34Z

blueocean-rest/src/main/java/io/jenkins/blueocean/rest/model/BlueRun.java

@@ -53,7 +56,7 @@

    /** Date String format */
    public static final String DATE_FORMAT_STRING = "yyyy-MM-dd'T'HH:mm:ss.SSSZ";
-
+    public static final DateTimeFormatter DATE_FORMAT = DateTimeFormat.forPattern("yyyy-MM-dd'T'HH:mm:ss.SSSZ");


Can we move joda specific code to AbstractRunImpl? This will also result in to blue-rest module not to depend on specific data time library.

vivek · 2017-08-14T18:44:11Z

blueocean-rest/pom.xml

@@ -23,6 +23,10 @@
          <groupId>io.jenkins.blueocean.rest.annotation</groupId>
          <artifactId>capability-annotation</artifactId>
      </dependency>
+      <dependency>


See my other comment, we should avoid depending on specific date time library in models.

vivek · 2017-08-16T01:16:35Z

...pi-impl/src/main/java/io/jenkins/blueocean/rest/impl/pipeline/OrganizationFolderRunImpl.java

@@ -97,6 +96,21 @@ public Date getEndTime() {
    }

    @Override
+    public String getStartTimeString() {


Maybe declare these as abstract? default implementation returning null can be misleading.

Ignore that comment, for some reason I mistook it as one of the model class. All good here:)

michaelneale · 2017-08-16T03:25:35Z

Not sure about the build failure, is this a test coverage failure?

[WARNING] Rule violated for bundle blueocean-rest-impl: classes missed count is 3.00, but expected maximum is 0.00

?? (jacoco seems annoying in every single way, except that it does enforce coverage levels)

vivek · 2017-08-16T04:22:16Z

@michaelneale it means there are 3 classes without test coverage. Reports are in that modules target/code-coverage/report directory. Guess time to write Jacoco HTML publisher:)

i386 · 2017-08-16T04:39:07Z

@vivek the only class I can see that does not have coverage is BlueJUnitTestResult. Would you mind taking a look? I am stumped :(

vivek

Other than stapler changes, rest of the code looks good. There was a minor comment I left to log error message in case of loading value from callable.

Maybe we should remove stapler enhancement from here if we don't want to wait for stapler PR to merge and deal with it in separate PR?

vivek · 2017-08-16T04:32:31Z

blueocean-commons/src/main/java/io/jenkins/blueocean/commons/stapler/export/Property.java

-                try {
-                    model = owner.get(d.getClass(), parent.type, name);
-                } catch (NotExportableException e) {
+                if (d.getClass().getAnnotation(ExportedBean.class) == null) {


Changes in Property.java in this PR is different from the ones in PR. I think its cleaner to remove changes from here and merge stapler changes in separate PR, after stapler PR is merged. This way we can merge this PR without waiting for stapler changes and get immediate perf benefits.

vivek · 2017-08-16T04:34:27Z

...pi-impl/src/main/java/io/jenkins/blueocean/rest/impl/pipeline/OrganizationFolderRunImpl.java

@@ -97,6 +96,21 @@ public Date getEndTime() {
    }

    @Override
+    public String getStartTimeString() {


Ignore that comment, for some reason I mistook it as one of the model class. All good here:)

vivek · 2017-08-16T04:44:50Z

...cean-rest-impl/src/main/java/io/jenkins/blueocean/service/embedded/rest/AbstractRunImpl.java

+                    }
+                }).orNull();
+            } catch (ExecutionException e) {
+                return null;


should log error message or else the checked exception thrown by value loaded callable will be silently ignored.

vivek · 2017-08-16T04:49:14Z

@i386 since we don't have coverage report archived, if you build that module locally (Where its reporting coverage error) you should be able to see the index.html (target/code-coverage/report/index.html) and see which classes lack coverage (it includes anonymous inner classes as well).

i386 · 2017-08-16T23:18:39Z

@vivek I have removed the stapler changes from here. We can add them back when jenkinsci/stapler#127 is merged.

vivek

LGTM 🐝

James Dumay added 9 commits August 14, 2017 15:12

Add a cache for expensive BlueTestSummary calculation

e4a7925

NotExportableException exception thrown in Model::ctor adding many mi…

3d0125d

…lliseconds to export

Branch metadata did not hit cache

3803224

Move branch issues onto the Branch model where it belongs

2ab30e5

It will be cached and the action lookup here wont happen

Remove up to 5ms per call off of constructing date SimpleDateFormat objs

f17af17

Use jodatime for a threadsafe datetimeformatter

cccef35

Found another 7ms to DateTimeFormatter construction

bef47fb

Here's a few more SimpleDateFormat usages in hot paths

f619102

Remove empty()

38a1b29

i386 requested a review from vivek August 14, 2017 06:57

i386 added the needs-review label Aug 14, 2017

i386 changed the title ~~Topic/micro opts~~ Optimizations Aug 14, 2017

i386 changed the title ~~Optimizations~~ Optimizations to REST API Aug 14, 2017

vivek requested changes Aug 14, 2017

View reviewed changes

Move jodatime usage to impl

51708cd

vivek reviewed Aug 16, 2017

View reviewed changes

i386 mentioned this pull request Aug 16, 2017

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

Merged

Move BlueTestSummary cache into RunImpl

f878a36

vivek reviewed Aug 16, 2017

View reviewed changes

James Dumay added 5 commits August 16, 2017 14:59

publishHTML

717cf53

LOL this fixes jacoco

e8eab73

Make same as stapler PR

fa90145

Revert Jenkinsfile change

9e80628

Fix not null build failure

5aa7adf

Remove stapler parts of micro opt

ae740f5

James Dumay added 3 commits August 17, 2017 10:13

Move branch metadata tests out to own class, fix tests

2e717f7

Ensure users checkForNull on pull request and branch fetch

a02d0ef

Invalidate caches

c34f393

vivek approved these changes Aug 17, 2017

View reviewed changes

James Dumay added 2 commits August 17, 2017 10:50

Fix findbugs... good thing i added @checkfornull here

5fb42e0

Remove non deterministic assert

9601175

i386 merged commit 15fff7a into master Aug 17, 2017

i386 deleted the topic/micro-opts branch October 9, 2017 02:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimizations to REST API #1323

Optimizations to REST API #1323

i386 commented Aug 14, 2017

i386 commented Aug 14, 2017

vivek commented Aug 14, 2017

vivek Aug 14, 2017

i386 Aug 16, 2017

vivek Aug 14, 2017

i386 Aug 16, 2017

vivek Aug 16, 2017

vivek Aug 14, 2017

vivek Aug 14, 2017

vivek Aug 16, 2017

i386 Aug 16, 2017

vivek Aug 16, 2017

michaelneale commented Aug 16, 2017

vivek commented Aug 16, 2017

i386 commented Aug 16, 2017

vivek left a comment

vivek Aug 16, 2017

vivek Aug 16, 2017

vivek Aug 16, 2017

vivek commented Aug 16, 2017 •

edited

Loading

i386 commented Aug 16, 2017 •

edited

Loading

vivek left a comment

Optimizations to REST API #1323

Optimizations to REST API #1323

Conversation

i386 commented Aug 14, 2017

Description

Submitter checklist

Reviewer checklist

i386 commented Aug 14, 2017

vivek commented Aug 14, 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

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

michaelneale commented Aug 16, 2017

vivek commented Aug 16, 2017

i386 commented Aug 16, 2017

vivek 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

vivek commented Aug 16, 2017 • edited Loading

i386 commented Aug 16, 2017 • edited Loading

vivek left a comment

Choose a reason for hiding this comment

vivek commented Aug 16, 2017 •

edited

Loading

i386 commented Aug 16, 2017 •

edited

Loading