Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Change the multipart lib to improve performances #2409

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Change the multipart lib to improve performances #2409

wants to merge 1 commit into from

Conversation

ClemPi
Copy link
Contributor

@ClemPi ClemPi commented Apr 8, 2016

Related to #2355 I reworked the Multipart handling and used https://github.com/Vodurden/Http-Multipart-Data-Parser to do so.

Here are some performances test I made:

before after
2GB 55s 33s
4GB 120s 75s
8GB 314s 285s

Please let me know if this look OK for you before I start working on the tests. I included all the lib, maybe we want to only pick relevant classes.

Regards

@thecodejunkie
Copy link
Member

Hi,

Nice! This is one of those things that you just don't pull in without giving it a proper code review and step the code a bit. Even then it's a nightmare to dare to pull the merge button because of the uncertainty of that scenarios that might be broken :D

So before we start reviewing that, there are some pretty big code style violations that would need to be fixed. I would prefer if they were addressed before we even started looking at the actual implementation logic. You can find the style guides here (they were also linked during the pull-request process)

Thanks

@ClemPi
Copy link
Contributor Author

ClemPi commented Apr 27, 2016

I'm aware that the code added in HttpMultipart directory is not coding style compliant.
As I said the code is coming from a lib, and I think it will be easier to maintain this kind of "external" dependency if we don't change the code too much. Looks like it's already the case for TinyIOC.

Regards

@aidapsibr
Copy link
Contributor

Rather than directly pull in the codebase, can we learn from it and apply changes to the existing handling?

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

Successfully merging this pull request may close these issues.

3 participants