Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve #printString implementations #4865

Conversation

@leonardoce
Copy link
Contributor

leonardoce commented Oct 9, 2019

On MalGraphEdge, MalHitsNode and MalWeightedEdge, the #printString
implementation was calling #printString on their ivars, causing the
creation of too many temporary Streams and spurious copies.

This patch reimplement those methods in a more performance-aware way,
while adding some unit tests that were running also before the
implementation to be modified, avoiding changing the former behavior of
the methods.

Fixes: #4855

@VincentBlondeau

This comment has been minimized.

Copy link
Contributor

VincentBlondeau commented Oct 9, 2019

Good job. But could you implement the printOn: aStream method instead of printString ? (and then the super class printString can be used instead of subclassing it)

@leonardoce

This comment has been minimized.

Copy link
Contributor Author

leonardoce commented Oct 9, 2019

Good job. But could you implement the printOn: aStream method instead of printString ? (and then the super class printString can be used instead of subclassing it)

Yes, it's a good idea. Let me modify the patch according to your suggestion.

@leonardoce leonardoce force-pushed the leonardoce:4855-printString-reimplementations-look-suspicious branch from 0882343 to 84f2586 Oct 9, 2019
@leonardoce

This comment has been minimized.

Copy link
Contributor Author

leonardoce commented Oct 9, 2019

@VincentBlondeau I modified my patch considering your advices. Thanks for them!

Copy link
Contributor

VincentBlondeau left a comment

Thanks for the changes! Still some to do and it is ok for me !

src/Moose-Algos-Graph/MalHitsNode.class.st Outdated Show resolved Hide resolved
src/Moose-Algos-Graph/MalHitsNode.class.st Outdated Show resolved Hide resolved
src/Moose-Algos-Graph/MalHitsNode.class.st Outdated Show resolved Hide resolved
On MalGraphEdge, MalHitsNode and MalWeightedEdge, the #printString
implementation was calling #printString on their ivars, causing the
creation of too many temporary Streams and spurious copies.

This patch replace those methods, when needed, with a #printOn:
implementation. In that way, we avoid creating substreams and spurious
copies.

Some unit tests have been added, and they were running also before the
implementation change, this is to ensure that the former behavior have
not been changed.

Fixes: #4855

Co-authored-by: Vincent Blondeau <vincent.blondeau@polytech-lille.net>
@leonardoce leonardoce force-pushed the leonardoce:4855-printString-reimplementations-look-suspicious branch from 84f2586 to 622bf4e Oct 9, 2019
@leonardoce

This comment has been minimized.

Copy link
Contributor Author

leonardoce commented Oct 9, 2019

@VincentBlondeau I've set you as co-author of my commit. If that's not ok with you, please feel free to remove the line from the commit message. Thanks!

@Ducasse Ducasse merged commit 66874bd into pharo-project:Pharo8.0 Oct 14, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/jenkins/pr-merge This commit cannot be built
Details
WIP Ready for review
Details
probot/minimum-reviews No pending reviews
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.