Skip to content

Conversation

@maciej-kisiel
Copy link

Instead of creating a serializer instance each time the BlockManager serialization methods are used, keep a pool of available serializers. It should improve performance.

@mccheah
Copy link
Contributor

mccheah commented Aug 21, 2015

Jenkins, test this please

@andrewor14
Copy link
Contributor

ok to test

@andrewor14
Copy link
Contributor

@maciej-kisiel have you done any benchmarking on this?

@SparkQA
Copy link

SparkQA commented Sep 1, 2015

Test build #41892 has finished for PR 8292 at commit a3bddec.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maciej-kisiel
Copy link
Author

@andrewor14 I'm working on it

@SparkQA
Copy link

SparkQA commented Sep 8, 2015

Test build #42138 has finished for PR 8292 at commit 492c06a.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maciej-kisiel
Copy link
Author

Weird, I just rebased it..

@andrewor14
Copy link
Contributor

Yeah master tests are flaky. retest this please

@SparkQA
Copy link

SparkQA commented Sep 10, 2015

Test build #42276 has finished for PR 8292 at commit 492c06a.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member

zsxwing commented Dec 11, 2015

@maciej-kisiel could you rebase this one?

@andrewor14
Copy link
Contributor

@maciej-kisiel any updates on this one? Do you still plan to work on this?

@maciej-kisiel
Copy link
Author

It will be difficult for me to benchmark this, because in the meantime I lost access to a big data environment.

@andrewor14
Copy link
Contributor

I can see how this patch can improve performance; currently we may end up creating a SerializerInstance per partition, which could be expensive if there are thousands of partitions. However, I don't know how much performance this could save in practice. If it doesn't end up buying us much then the extra complexity here is not worth it.

@srowen
Copy link
Member

srowen commented Dec 16, 2015

Agree, do you mind closing this PR?

@asfgit asfgit closed this in ce5fd40 Dec 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants