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

_PyStaticCode_InternStrings is called ~800 times during startup. #96459

Open
markshannon opened this issue Aug 31, 2022 · 9 comments
Open

_PyStaticCode_InternStrings is called ~800 times during startup. #96459

markshannon opened this issue Aug 31, 2022 · 9 comments
Labels
3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage topic-subinterpreters type-bug An unexpected behavior, bug, or error

Comments

@markshannon
Copy link
Member

The _Py_Deepfreeze_Init() function calls _PyStaticCode_InternStrings once for each string.

Creating the interned string dict in one go, from the table of static strings would be much faster.
something like this (error checking omitted, and other liberties taken for brevity)

_Py_CreateInternedDict(PyObject **strs, int n) {
   PyDictKeys *table = make_dict_keys(n);
   for (int i = 0; i < n; i++) {
        table->entries[i].key = table->entries[i].value = strs[i];
        table->entries[i].hashcode = strs[i].hash_code;
        // Fix up size and entries here.
   }
   return new_dict(table);
}

All the hash codes will need to be initialized first, but we probably should do that anyway.

@markshannon markshannon added the performance Performance or resource usage label Aug 31, 2022
@markshannon
Copy link
Member Author

@gvanrossum
Copy link
Member

Maybe @kumaraditya303 is interested in doing this?

@ericsnowcurrently
Copy link
Member

Yeah, seems like a good improvement. FWIW, I considered doing this a while back but with a static initializer but it was way too messy.

@gvanrossum gvanrossum added the triaged The issue has been accepted as valid by a triager. label Sep 1, 2022
@kumaraditya303
Copy link
Contributor

What percentage of the startup _PyStaticCode_InternStrings accounts for? @ericsnowcurrently Do you have a flame graph of startup with deepfreeze turned on based on recent main?

@gvanrossum
Copy link
Member

All the hash codes will need to be initialized first, but we probably should do that anyway.

This is a bit problematic since the hash codes vary based on the hash seed. I wouldn’t be surprised if this was most of the cost. Please look into that before writing the code.

@markshannon
Copy link
Member Author

The hashes always get initialized, it will be no slower to do them all up front.

@kumaraditya303
Copy link
Contributor

Another thing is if we create a table of strings it should also include the global identifier strings not just deepfreeze strings.

@kumaraditya303 kumaraditya303 added the 3.12 bugs and security fixes label Sep 5, 2022
@gvanrossum
Copy link
Member

The hashes always get initialized, it will be no slower to do them all up front.

My point was that we don't know if this rearrangement of the code is going to have any effect. And in particular if the hash computation is expensive, it may dominate the cost, so rearranging the code won't help. So yes, we need a flame graph or something like that to show where the time is spent.

@AraHaan
Copy link
Contributor

AraHaan commented Sep 5, 2022

I agree with listing the strings so then allocations can be faster.

@iritkatriel iritkatriel added interpreter-core (Objects, Python, Grammar, and Parser dirs) and removed triaged The issue has been accepted as valid by a triager. labels Nov 27, 2023
@ericsnowcurrently ericsnowcurrently added type-bug An unexpected behavior, bug, or error topic-subinterpreters labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage topic-subinterpreters type-bug An unexpected behavior, bug, or error
Projects
Status: Todo
Development

No branches or pull requests

6 participants