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

Prevent memory leak in CandleStickChart & VolumeChart #3914

Merged
merged 1 commit into from Jan 27, 2020

Conversation

@stejbac
Copy link
Contributor

stejbac commented Jan 22, 2020

Ensure that the superclass methods XYChart.removeDataItemFromDisplay and XYChart.removeSeriesFromDisplay are always called from the implemented dataItemRemoved and seriesRemoved methods respectively, as specified by the API javadoc.

This prevents a leak of old Candle and VolumeBar objects every time the trades charts view is updated. The former is quite substantial, as each Candle object has a retained size of about 70kB and there are up to 90 candlesticks / volume bars leaked per chart update.

--

By cycling through the Month, Week and Day interval a few dozen times, the following heap usage stats were obtained using JProfiler:

Screenshot from 2020-01-22 10-42-19

I believe updates to the two charts happen periodically as long as the trades charts view is selected, as it appeared to leak while unattended as well. I haven't had a chance to measure the rate, though.

@stejbac stejbac requested review from ripcurlx and sqrrm as code owners Jan 22, 2020
Ensure that the superclass methods XYChart.removeDataItemFromDisplay &
XYChart.removeSeriesFromDisplay are always called from the implemented
dataItemRemoved & seriesRemoved methods respectively, as specified by
the API javadoc.

This prevents a leak of old Candle & VolumeBar objects every time the
trades charts view is updated. The former is quite substantial, as each
Candle object has a retained size of about 70kB and there are up to 90
candlesticks / volume bars leaked per chart update.
@stejbac stejbac force-pushed the stejbac:fix-trades-charts-memory-leak branch from ab2ad33 to 7b7ea64 Jan 22, 2020
Copy link
Member

ripcurlx left a comment

ACK

Tested PR against Mainnet switching between time frames and currencies and everything seems to be still working as before from a user perspective. Code changes looking fine to me as well, but I didn't verified the change using jProfiler. If anyone wants to review/verify this part as well feel free to do so.

@ripcurlx ripcurlx merged commit 04868cd into bisq-network:master Jan 27, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ripcurlx ripcurlx added this to the v1.2.6 milestone Jan 29, 2020
@stejbac stejbac mentioned this pull request Feb 10, 2020
@ripcurlx ripcurlx mentioned this pull request Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.