Open ggrossetie opened 3 years ago
ggrossetie
commented
3 years ago Using an ArrayList instead of an HashSet divide time by 2.5.
I didn't spot any visual difference and other diagrams seem to run as fast as before.
If needed, we could probably remove duplicates once seedFillOld is completed?
stathissideris
commented
3 years ago Hello, thanks for looking into this. By this point I'm also unfamiliar with the code (haven't touched it for ages!) but I'm wondering whether seedFill is equivalent to seedFillOld but faster. seedFill is not being used in the code at all, and the naming implies that I was going to replace the "old" method with the new one. Would you like to switch to the new one and see if it's correct/faster?
ggrossetie
commented
3 years ago Hello @stathissideris!
Thanks for you reply.
When using seedFill the process seems to run in an infinite loop (inside fillCellsWith) 😬
But even if the implementation was correct, I doubt it would be any faster because what takes most of the time is HashSet.add and this line is present in both implementations:
Using TreeSet is considerably faster (17 seconds).
Since elements in a TreeSet are sorted, I was "forced" to implement Comparable:
@Override
public int compareTo(Cell o) {
if (this.x == o.x) {
return this.y - o.y;
}
return this.x - o.x;
}Using ArrayList is still faster (10 seconds) but can contains duplicates.
Do you remember why/if using a Set is important in this context?
ggrossetie
commented
3 years ago Hello, thanks for looking into this. By this point I'm also unfamiliar with the code (haven't touched it for ages!)
@stathissideris I wonder if you would like to hand over or add more contributors to this project?
Pepijn has done great work (and fixed a couple of issues) on his fork: https://github.com/pepijnve/ditaa
PlantUML is using ditaa but there are some major issues (infinite loops, exceptions, inefficient code) that can be used as a Denial of Service (DoS) attack. So if you don't plan to work on ditaa anymore maybe we should plan the transition so that PlantUML can migrate to an active fork? What do you think?
hohle
commented
1 year ago I understand this won't be merged, but the HashSet isn't really the issue, the #hashCode method TextGrid.Cell is. I changed the implementation to Szudzik's Elegant Pairing and saw significant speedup. I haven't looked at Pepijn's branch yet, but I'm also accumulating a bunch of refactoring and performance enhancements in a local branch that I may push sometime in the future.
The following diagram takes more than 30 seconds to produce an image:
Here's the flamegraph:
As you can see 80% of the time is spent in the
TextGrid.seedFillOldmethod:https://github.com/stathissideris/ditaa/blob/f2286c4ddbb4f80e04196be09f4775d227fc9b87/src/java/org/stathissideris/ascii2image/text/TextGrid.java#L1354-L1383
I'm not familiar with the code base but maybe it can be optimized?
Let me know if you need more information, I will gladly provide them 🤗