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

Add tunable buffer size for spt3g filtering_istream #194

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Conversation

tskisner
Copy link
Member

This adds a small (and perhaps temporary) patch to specify the buffer size to the filtering_istream stack. This is a quick update to fix slow data loading and #192 will be rebased after this is merged. Also added GSL to the wheel build dependencies.

For testing on a compute node on perlmutter.nersc.gov, I used a single process and 8 threads. I then loaded one wafer (ufm_mv12) of satp3 observation obs_1714550584_satp3_1111111.

I used the new SO3G_FILESYSTEM_BUFFER environment variable to try a variety of buffer sizes loading this same data from the CFS and scratch filesystems. Note that the frame files in this case are each ~200MB, so the largest buffer size tested is actually larger than the frame files. The smaller values of buffer size were essentially unusable on CFS.

Here is a plot of the times:
spt3g_load

Note that CFS access is faster from a login node than a compute node, and scratch access is faster from a compute node than a login node. Here is the data in the plot:

Scratch Filesystem
-----------------------------------------------
Buffer Bytes        Time
0                    24.7
10                   6.71
100                  3.97
1000                 3.72
10000                3.6
100000               3.6     
1000000              3.6
10000000             3.6
20971520             3.7  (new default)  (13.8 on login)
100000000            3.8
1000000000          11.8

CFS Filesystem
-----------------------------------------------
Buffer Bytes        Time
0           
10          
100         
1000        
10000               523.0
100000              212.5
1000000              41.8
10000000             21.5
20971520             18.9  (new default)  (4.9 on login)
100000000            17.5
1000000000           24.8

@tskisner tskisner assigned mhasself and unassigned mhasself Dec 12, 2024
@tskisner tskisner requested a review from mhasself December 12, 2024 06:19
Copy link
Member

@mhasself mhasself left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a miracle... no notes. This basically resolves the performance concerns, as far as I can tell. Let's get it out there.

@arahlin
Copy link

arahlin commented Dec 12, 2024

We'll just integrate this upstream as a keyword argument to the g3_istream / G3Reader code.

@nwhitehorn
Copy link

I'm not really sure there is much downside to setting a big global default rather than having a config parameter -- no one using this software is reading a couple KB and then going home and especially no one is reading a couple KB from something really slow where they would notice the extra reads.

@tskisner tskisner merged commit e30cca2 into master Dec 12, 2024
11 checks passed
@tskisner tskisner deleted the buffer_tune branch December 12, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants