Dto streaming #3103

tareksha · 2016-11-17T11:33:32Z

What does this PR do?

Eliminates the redundant creation of full in-memory JSON tress and then strings when reading DTOs from requests or writing them to responses. Applies to general consumption of DtoFactory as well, not JAX-RS services only.

What issues does this PR fix or reference?

Performance.
Current method always constructs a full replica of the DTO objects as an in-memory JsonElement tree, and then asks Gson to dump the tree as a full string. There are two issues here:
- Gson can generate the JSON in a stream-based method based on JsonWriter which eliminates the need for any intermediate trees. Actually no JSON elements are created at all here.
- The whole method of generating/parsing full JSON texts is not needed, the JAX-RS CheJsonProvider passes stream Reader and response Writer that can used directly by Gson, eliminating even the need of an intermediate full JSON texts.
Maintainability.
A large part of the generated DTO implementation code is there to replicate the DTO data in JsonElement trees, bit by bit. The code section in the generator that takes care of generating this java code in the tool is the most complicated area in the already hard to maintain generator. Gson can do this automatically and more efficiently (previous point).

Previous behavior

Full JSON trees built when converting DTOs to JSONs.
Full JSON text is created before parsing DTOs from JAX-RS requests and (vise versa for responses).

New behavior

JSON trees eliminated by using Gson to generate the DTOs objects directly.
Full JSON texts eliminated by using request Readers and response Writers directly in DtoFactory.
Smaller files generated for DTO implementations: workspace API reduced by 30%, machine API by 29%

Changelog

Removes redundant in-memory JSON trees for faster DTO parsing performance.

Signed-off-by: Tareq Sharafy tareq.sha@gmail.com

skabashnyuk · 2016-11-17T13:41:45Z

Hello @tareqhs thanks for this PR. Do you have any statistic that confirms performance optimization?

codenvy-ci · 2016-11-17T14:00:07Z

Build # 1052 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1052/ to view the results.

tareksha · 2016-11-17T17:35:05Z

Hi @skabashnyuk ,
Here are my benchmark results:

Setup
Prepare a random WorkspaceDto, composed of:

10 ProjectConfigDto, each composed of
- 10-character name
- 10-character type
- 10-character path
- 10 Link entries, each with a short name and 118-character command line
10 CommandDto, each composed of
- 10-character name
- 10-character type
- 5 simple attributes
3 EnvironmentDto, each composed of
- 3 ExtendedMachineDto, each with 5 attributes

All times below are in milliseconds.

Benchmark 1
Prepare a reusable Writer that has zero writing overhead (similar to /dev/null).
START TIMER
Call JsonSerializable.toJson(Writer) on the test DTO 20,000 times.
STOP TIMER
Because JsonSerializable.toJson(Writer) was introduced in the fix, the non-fixed benchmark actually does writer.write(dtoObj.toJson())

Without fix : 6601, 5569, 5931, 5479, 5486, 5838, 5523, 5556, 5521, 5495
With fix : 2945, 2714, 2692, 2890, 2704, 2738, 2700, 2705, 2724, 2756

Omitting the bootstrap pass, average benchmark time is reduced by 52%

Benchmark 2
Convert the full DTO to a JSON. Resulting text is made of 42,320 characters.
Open 20,000 UTF-8 streams that read the same content, all in-memory.
START TIMER
Call DtoFactory.createDtoFromJson(InputStream,Class) on all the open streams, one after another.
STOP TIMER

Without fix : 6758, 6460, 6480, 6492, 6602, 6455, 6431, 6478, 6400, 6617
With fix : 2533, 2361, 2337, 2301, 2347, 2294, 2300, 2367, 2451, 2474

Omitting the bootstrap pass, average benchmark time is reduced by 63%

* use Reader/Writer directly in DtoFactory, do not create full JSON texts * read/write DTOs using direct streaming methods instead of building trees * configure Gson type adapters instead of generating tree building code Signed-off-by: Tareq Sharafy <tareq.sha@gmail.com>

codenvy-ci · 2016-11-18T01:33:26Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1062/

tareksha · 2016-11-27T09:25:06Z

hi @skabashnyuk , any update on this ?

codenvy-ci · 2016-11-29T16:14:27Z

Build # 1149 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1149/ to view the results.

skabashnyuk · 2016-12-01T09:10:07Z

ci-build

codenvy-ci · 2016-12-01T10:42:33Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1179/

tareksha · 2016-12-01T13:42:33Z

@skabashnyuk seems the ci passes now

codenvy-ci · 2016-12-21T01:05:56Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1394/

skabashnyuk · 2016-12-22T12:16:21Z

core/che-core-api-dto/src/main/java/org/eclipse/che/dto/generator/DtoImpl.java

@@ -176,6 +177,16 @@ protected String getJsonFieldName(Method getterMethod) {
        return new ArrayList<>(getters.values());
    }

+    protected Set<String> getSuperGetterNames(Class<?> dto) {


skabashnyuk · 2016-12-22T12:17:49Z

core/che-core-api-dto/src/test/java/org/eclipse/che/dto/definitions/DTOHierarchy.java

@@ -46,5 +46,18 @@
        void setParentField(String parentField);

        ChildDto withParentField(String parentField);
+


What is the purpose of changes in DTOHierarchy class?

shadowed fields cause caused initial failure, fixed this and added them to make the behavior is consistent
@skabashnyuk

Can you elaborate a bit more. explanation is not clear for me.

Gson fails if a class has a shadowed field (a field with the same name as another field in a base class). To avoid this shadowed fields are omitted from generated classes and this test verifies the behavior. @skabashnyuk

codenvy-ci · 2016-12-24T11:07:20Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1454/

tareksha · 2017-01-12T13:41:35Z

hi @skabashnyuk , any update on this ?

skabashnyuk · 2017-01-12T13:53:28Z

No updates. I'm looking for some free time-frame to make some integration and performance testing internally. Recently we made some performance testing of ws-agent and one of the slowest part was DTO parsing codenvy/codenvy#1466. Interesting how your PR can affect on this.

skabashnyuk · 2017-01-12T13:53:59Z

Take a look on https://cloud.githubusercontent.com/assets/1614429/21721914/fd47cc40-d432-11e6-827b-4e8c287fc75d.png

bmicklea · 2017-01-31T19:52:03Z

Added Changlog entry.

down-port eclipse-che/che#3103 Change-Id: I1465e8b1023511e9ce248340c45f3cf9eb957157

down-port eclipse-che/che#3103 Change-Id: Iea2704e625105ad483ee9f1348b217b0bc493d6e Signed-off-by: Tareq Sharafy <tareq.sha@gmail.com>

* Streaming approach for DTO serialization * use Reader/Writer directly in DtoFactory, do not create full JSON texts * read/write DTOs using direct streaming methods instead of building trees * configure Gson type adapters instead of generating tree building code Signed-off-by: Tareq Sharafy <tareq.sha@gmail.com> * add missing javadoc

tareksha mentioned this pull request Nov 17, 2016

Rework dto generator with string templates #2242

Closed

tareksha force-pushed the dto_streaming branch from 7dc41f6 to d7d5661 Compare November 17, 2016 16:41

tareksha force-pushed the dto_streaming branch from d7d5661 to f89f313 Compare November 17, 2016 22:22

benoitf force-pushed the master branch from f0ee59a to aba9a87 Compare December 20, 2016 09:31

skabashnyuk reviewed Dec 22, 2016

View reviewed changes

add missing javadoc

cc7c1c2

tareksha mentioned this pull request Jan 12, 2017

Investigate the ways to measure bootstrap performance using jvisualvm for codenvy in docker codenvy/codenvy#1466

Closed

skabashnyuk added status/open-for-dev An issue has had its specification reviewed and confirmed. Waiting for an engineer to take it. team/platform sprint/next kind/task Internal things, technical debt, and to-do tasks to be performed. labels Jan 17, 2017

skabashnyuk mentioned this pull request Jan 18, 2017

Performance optimization activity #3792

Closed

7 tasks

TylerJewell mentioned this pull request Jan 21, 2017

Better DTO Generation #210

Closed

skabashnyuk added sprint/current and removed sprint/next labels Jan 23, 2017

mkuznyetsov merged commit ac876e3 into eclipse-che:master Jan 31, 2017

mkuznyetsov removed the status/open-for-dev An issue has had its specification reviewed and confirmed. Waiting for an engineer to take it. label Jan 31, 2017

skabashnyuk added this to the 5.2.0 milestone Jan 31, 2017

tareksha deleted the dto_streaming branch February 1, 2017 06:00

JamesDrummond mentioned this pull request Feb 6, 2017

5.2 Milestone Overview #3976

Closed

9 tasks

tareksha added a commit to tareksha/che-core that referenced this pull request Feb 13, 2017

DTO streaming

6ac419e

down-port eclipse-che/che#3103 Change-Id: I1465e8b1023511e9ce248340c45f3cf9eb957157

tareksha added a commit to tareksha/che-core that referenced this pull request Feb 13, 2017

DTO streaming

693320f

down-port eclipse-che/che#3103 Change-Id: Iea2704e625105ad483ee9f1348b217b0bc493d6e Signed-off-by: Tareq Sharafy <tareq.sha@gmail.com>

tareksha mentioned this pull request Feb 13, 2017

DTO streaming codenvy-legacy/che-core#745

Merged

tareksha added a commit to tareksha/che-core that referenced this pull request Feb 13, 2017

DTO streaming

0b4115f

down-port eclipse-che/che#3103 Change-Id: Iea2704e625105ad483ee9f1348b217b0bc493d6e Signed-off-by: Tareq Sharafy <tareq.sha@gmail.com>

tareksha added a commit to tareksha/che-core that referenced this pull request Feb 13, 2017

DTO streaming

101abb8

down-port eclipse-che/che#3103 Change-Id: Iea2704e625105ad483ee9f1348b217b0bc493d6e Signed-off-by: Tareq Sharafy <tareq.sha@gmail.com>

tareksha added a commit to tareksha/che-core that referenced this pull request Feb 13, 2017

DTO streaming

eccdb6d

down-port eclipse-che/che#3103 Change-Id: Iea2704e625105ad483ee9f1348b217b0bc493d6e Signed-off-by: Tareq Sharafy <tareq.sha@gmail.com>

tareksha added a commit to tareksha/che-core that referenced this pull request Feb 13, 2017

DTO streaming

0e68066

down-port eclipse-che/che#3103 Change-Id: Iea2704e625105ad483ee9f1348b217b0bc493d6e Signed-off-by: Tareq Sharafy <tareq.sha@gmail.com>

tareksha added a commit to codenvy-legacy/che-core that referenced this pull request Feb 13, 2017

DTO streaming (#745)

bf498ea

down-port eclipse-che/che#3103 Change-Id: Iea2704e625105ad483ee9f1348b217b0bc493d6e Signed-off-by: Tareq Sharafy <tareq.sha@gmail.com>

jonahgraham mentioned this pull request Feb 15, 2018

CHE-8557: No Dto available for FormattingOptions #8784

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dto streaming #3103

Dto streaming #3103

tareksha commented Nov 17, 2016 •

edited by JamesDrummond

Loading

skabashnyuk commented Nov 17, 2016 •

edited

Loading

codenvy-ci commented Nov 17, 2016

tareksha commented Nov 17, 2016 •

edited

Loading

codenvy-ci commented Nov 18, 2016

tareksha commented Nov 27, 2016

codenvy-ci commented Nov 29, 2016

skabashnyuk commented Dec 1, 2016

codenvy-ci commented Dec 1, 2016

tareksha commented Dec 1, 2016

codenvy-ci commented Dec 21, 2016

skabashnyuk Dec 22, 2016

skabashnyuk Dec 22, 2016

tareksha Dec 24, 2016 •

edited

Loading

skabashnyuk Dec 25, 2016

tareksha Dec 26, 2016 •

edited

Loading

codenvy-ci commented Dec 24, 2016

tareksha commented Jan 12, 2017

skabashnyuk commented Jan 12, 2017

skabashnyuk commented Jan 12, 2017

bmicklea commented Jan 31, 2017

		@@ -46,5 +46,18 @@
		void setParentField(String parentField);

		ChildDto withParentField(String parentField);

Dto streaming #3103

Dto streaming #3103

Conversation

tareksha commented Nov 17, 2016 • edited by JamesDrummond Loading

What does this PR do?

What issues does this PR fix or reference?

Previous behavior

New behavior

Changelog

skabashnyuk commented Nov 17, 2016 • edited Loading

codenvy-ci commented Nov 17, 2016

tareksha commented Nov 17, 2016 • edited Loading

codenvy-ci commented Nov 18, 2016

tareksha commented Nov 27, 2016

codenvy-ci commented Nov 29, 2016

skabashnyuk commented Dec 1, 2016

codenvy-ci commented Dec 1, 2016

tareksha commented Dec 1, 2016

codenvy-ci commented Dec 21, 2016

skabashnyuk Dec 22, 2016

Choose a reason for hiding this comment

skabashnyuk Dec 22, 2016

Choose a reason for hiding this comment

tareksha Dec 24, 2016 • edited Loading

Choose a reason for hiding this comment

skabashnyuk Dec 25, 2016

Choose a reason for hiding this comment

tareksha Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

codenvy-ci commented Dec 24, 2016

tareksha commented Jan 12, 2017

skabashnyuk commented Jan 12, 2017

skabashnyuk commented Jan 12, 2017

bmicklea commented Jan 31, 2017

tareksha commented Nov 17, 2016 •

edited by JamesDrummond

Loading

skabashnyuk commented Nov 17, 2016 •

edited

Loading

tareksha commented Nov 17, 2016 •

edited

Loading

tareksha Dec 24, 2016 •

edited

Loading

tareksha Dec 26, 2016 •

edited

Loading