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

printString reimplementations look suspicious #4855

Open
Ducasse opened this issue Oct 7, 2019 · 1 comment · May be fixed by #4865

Comments

@Ducasse
Copy link
Member

commented Oct 7, 2019

We should revisit the printString implementations because it looks like some of them are simply printOn: disguised.

In addition some of them promote bad pattern:

MalGraphEdge >> printString

^ self from printString, ' -> ', self to printString

Why do we create two substreams instead of sharing the printString via printOn:?
Here printString each time create a stream then .... it is worse , create spurious copies...
Super super ugly.

In general printOn: in general should not call printString, better use self print: str
So may be we can open another issue for this issue.

@Ducasse Ducasse changed the title printString reimplementation look suspicious printString reimplementations look suspicious Oct 7, 2019
leonardoce added a commit to leonardoce/pharo that referenced this issue 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: pharo-project#4855
leonardoce added a commit to leonardoce/pharo that referenced this issue 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: pharo-project#4855
@leonardoce leonardoce referenced a pull request that will close this issue Oct 9, 2019
@Ducasse

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

Thanks this is cool :)

leonardoce added a commit to leonardoce/pharo that referenced this issue 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 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: pharo-project#4855
leonardoce added a commit to leonardoce/pharo that referenced this issue 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 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: pharo-project#4855

Co-authored-by: Vincent Blondeau <vincent.blondeau@polytech-lille.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.