Skip to content
This repository has been archived by the owner on Nov 16, 2020. It is now read-only.

Speed up TemplateDataEncoder significantly. #50

Merged
merged 6 commits into from
Apr 25, 2019

Conversation

MrMage
Copy link
Contributor

@MrMage MrMage commented Apr 1, 2019

Results:

  • Before the optimizations, each invocation inside measure takes about 4s on my Core i7-6700k.
  • After conforming Data to TemplateDataRepresentable, each invocation inside measure takes about 0.2s.
  • After rewriting TemplateDataEncoder, the runtime halves again to 0.1s. (NestedData is fairly inefficient, especially for "deeper" hierarchies.)

Screenshot (including the improvements from vapor/core#199):
Screen Shot 2019-04-01 at 11 12 50

Results:

- Before the optimizations, each invocation inside `measure` takes about 4s on my Core i7-6700k.
- After conforming `Data` to `TemplateDataRepresentable`, each invocation inside `measure` takes about 0.2s.
- After rewriting `TemplateDataEncoder`, the runtime halves again to 0.1s. (`NestedData` is fairly inefficient, especially for "deeper" hierarchies.)
MrMage added a commit to MrMage/core that referenced this pull request Apr 1, 2019
…ten`.

This further halves the runtime in vapor/template-kit#50 from 0.1s to 50ms.
MrMage added a commit to MrMage/core that referenced this pull request Apr 1, 2019
…ten`.

This further halves the runtime in vapor/template-kit#50 from 0.1s to 50ms.
tanner0101 pushed a commit to vapor/core that referenced this pull request Apr 3, 2019
…ten (#199)

* Significantly improve the performance of `Collection<FutureType>.flatten`.

This further halves the runtime in vapor/template-kit#50 from 0.1s to 50ms.

* Add a benchmark for `flatten`.
@tanner0101 tanner0101 added the enhancement New feature or request label Apr 12, 2019
tanner0101
tanner0101 previously approved these changes Apr 12, 2019
Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. It looks like a similar method to what I've been using for Vapor 4's encoders. See this one for example: https://github.com/vapor/leaf/pull/139/files#diff-a1f7fd8772e8db228f62e4a0b303ee06R1

@tanner0101
Copy link
Member

Looks like there is a CI failure:

/root/project/Tests/TemplateKitTests/TemplateDataEncoderTests.swift:136:27: error: method does not override any method from its superclass
            override func encode(to encoder: Encoder) throws {

@MrMage
Copy link
Contributor Author

MrMage commented Apr 15, 2019

Looks like there is a CI failure:

/root/project/Tests/TemplateKitTests/TemplateDataEncoderTests.swift:136:27: error: method does not override any method from its superclass
            override func encode(to encoder: Encoder) throws {

Thanks for spotting that! I've updated the tests now; hopefully this will be fixed now.

@MrMage
Copy link
Contributor Author

MrMage commented Apr 17, 2019

Weird; on Mac the override keyword is required, and the tests pass with it. On Linux, it must not be used, and the tests fail without it...

@MrMage
Copy link
Contributor Author

MrMage commented Apr 17, 2019

Added a manual implementation of super.encode now, which should work on both Mac and Linux.

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @MrMage !

@tanner0101 tanner0101 merged commit 3e6d37e into vapor:master Apr 25, 2019
@penny-coin
Copy link

Hey @MrMage, you just merged a pull request, have a coin!

You now have 26 coins.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants