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

[performance] loading VOTable in CARTA requires more RAM comparing to TOPCAT #1217

Closed
kswang1029 opened this issue Nov 17, 2022 · 11 comments · Fixed by #1228
Closed

[performance] loading VOTable in CARTA requires more RAM comparing to TOPCAT #1217

kswang1029 opened this issue Nov 17, 2022 · 11 comments · Fixed by #1228
Assignees
Labels
question Further information is requested R&D

Comments

@kswang1029
Copy link
Contributor

kswang1029 commented Nov 17, 2022

Based on the psrecord test, loading a VOTable in CARTA requires more RAM comparing to TOPCAT. CARTA is faster than TOPCAT but it has a stricter memory requirement. It would be good to study a bit on how to reduce the memory usage while retaining the excellent performance.

Screen Shot 2022-11-17 at 13 06 49

@kswang1029 kswang1029 added question Further information is requested R&D labels Nov 17, 2022
@veggiesaurus
Copy link
Collaborator

I think this is the easiest solution: https://pugixml.org/docs/manual.html#dom.memory.compact, but we would need to add pugixml as a third-party lib instead of an installed dependency.

@kswang1029
Copy link
Contributor Author

kswang1029 commented Nov 17, 2022

So no way we can provide both modes (yes I am greedy) at the same time, right? (as a user configurable)

@veggiesaurus
Copy link
Collaborator

So no way we can provide both modes (yes I am greedy) at the same time, right? (as a user configurable)

it would be rather complicated. We'd need to build the library twice and muck around with linking stuff I think

@kswang1029
Copy link
Contributor Author

So no way we can provide both modes (yes I am greedy) at the same time, right? (as a user configurable)

it would be rather complicated. We'd need to build the library twice and muck around with linking stuff I think

I see. Then let's see if the performance is still good enough with PUGIXML_COMPACT.

@kswang1029
Copy link
Contributor Author

@jolopezl would you be able to investigate this with the suggested build flag and make comparisons without that flag?

@jolopezl
Copy link
Contributor

jolopezl commented Dec 4, 2022

PUGIXML_COMPACT seems to have the desired impact, halving the amount of memory usage:

PUGIXML_COMPACT

The changes are minimal: just adding pugixml as a third-party library and forcing the PUGIXML_COMPAT variable ON. I don't observe any significant performance detriment, but I would prefer some e2e test checked to assert this strongly.

@kswang1029
Copy link
Contributor Author

PUGIXML_COMPACT seems to have the desired impact, halving the amount of memory usage:

PUGIXML_COMPACT

The changes are minimal: just adding pugixml as a third-party library and forcing the PUGIXML_COMPAT variable ON. I don't observe any significant performance detriment, but I would prefer some e2e test checked to assert this strongly.

based on the plots, it seems with the PUGIXML_COMPACT flag ON, it uses less ram (~50% less) and the e2e time is also reduced? When the PUGIXML_COMPACT flag is OFF, do you see your OS is swapping? If so that might explain the e2e time difference.

@jolopezl
Copy link
Contributor

jolopezl commented Dec 5, 2022

@kswang1029, a small update with better CPU sampling:

PUGIXML_COMPACT

based on the plots, it seems with the PUGIXML_COMPACT flag ON, it uses less ram (~50% less) and the e2e time is also reduced? When the PUGIXML_COMPACT flag is OFF, do you see your OS is swapping? If so that might explain the e2e time difference.

The rectangles on the plots have the same width, so I'd say that roughly there's the same e2e time. For the case PUGIXML_COMPAT=OFF, I can effectively observe some swapping from 250MB up to 1GB (from several trials), so I would say yes, there are some OS-dependent effects.

@veggiesaurus
Copy link
Collaborator

@jolopezl which branch is this with? And which file? I'd like to try with my machine and ubuntu

@kswang1029
Copy link
Contributor Author

@kswang1029, a small update with better CPU sampling:

PUGIXML_COMPACT

based on the plots, it seems with the PUGIXML_COMPACT flag ON, it uses less ram (~50% less) and the e2e time is also reduced? When the PUGIXML_COMPACT flag is OFF, do you see your OS is swapping? If so that might explain the e2e time difference.

The rectangles on the plots have the same width, so I'd say that roughly there's the same e2e time. For the case PUGIXML_COMPAT=OFF, I can effectively observe some swapping from 250MB up to 1GB (from several trials), so I would say yes, there are some OS-dependent effects.

The "e2e" time should count between the initial 0% CPU usage and the final 0% CPU usage, not just duration of the 400% CPU usage. As your OS was swapping, it won't be a fair comparison of the e2e time between the two tests. I will perform some tests here.

@kswang1029
Copy link
Contributor Author

kswang1029 commented Dec 13, 2022

@jolopezl @veggiesaurus Here is a comparison with and without the PUGIXML_COMPACT=ON flag (jolopezl/1217_pugixml_compact vs dev)

This is tested with a desktop having 64GB of RAM. No swapping is observed during the test.

Screen Shot 2022-12-13 at 15 26 35

For the backend execution time, we see similar results. With PUGIXML_COMPACT=ON, it is very slightly slower but not that significant (sampling rate is 0.02s so the difference should be real).

For the peak RAM usage (excluding the RAM usage for the image itself), it is reduced by 50% roughly. Note that the image itself occupies 267MB.

For the final RAM usage (excluding the RAM usage for the image itself), it is reduced by 75% roughly. Note that the image itself occupies 267MB.

Based on this, it is a promising and significant improvement. 👍 @jolopezl please file a PR and I will do some other manual tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested R&D
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants