Redundant Py_CHARMASK called in some files

[qigangxu]

BPO	37752
Nosy	@vstinner, @ezio-melotti, @zooba, @animalize, @shihai1991, @qigangxu
PRs	bpo-37752: Delete redundant Py_CHARMASK in normalizestring() #15093 bpo-37752: Delete redundant Py_CHARMASK in normalizestring() #15093 bpo-37752: Delete redundant Py_CHARMASK in normalizestring() #15095

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-09-10.16:04:27.039>
created_at = <Date 2019-08-03.12:10:17.664>
labels = ['3.9', 'expert-unicode', 'performance']
title = 'Redundant Py_CHARMASK called in some files'
updated_at = <Date 2019-09-10.16:04:27.038>
user = 'https://github.com/qigangxu'

bugs.python.org fields:

activity = <Date 2019-09-10.16:04:27.038>
actor = 'steve.dower'
assignee = 'none'
closed = True
closed_date = <Date 2019-09-10.16:04:27.039>
closer = 'steve.dower'
components = ['Unicode']
creation = <Date 2019-08-03.12:10:17.664>
creator = 'qigangxu'
dependencies = []
files = []
hgrepos = []
issue_num = 37752
keywords = ['patch', 'patch']
message_count = 14.0
messages = ['348955', '348957', '348960', '348969', '348970', '348971', '348973', '349047', '349102', '349288', '349297', '349298', '351717', '351718']
nosy_count = 6.0
nosy_names = ['vstinner', 'ezio.melotti', 'steve.dower', 'malin', 'shihai1991', 'qigangxu']
pr_nums = ['15093', '15093', '15095']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue37752'
versions = ['Python 3.9']

[qigangxu]

In normalizestring(),
            ch = Py_TOLOWER(Py_CHARMASK(ch));
Where Py_TOLOWER is defined as following,
            #define Py_TOLOWER(c) (_Py_ctype_tolower[Py_CHARMASK(c)])

Redundant Py_CHARMASK called here.

[shihai1991]

Good catch ;), try to fix it, thanks.

[shihai1991]

Looks the PR is confused. Make sure: one PR on one dev branch.

[qigangxu]

Thanks Hai, for your careful check. There is a misoperation in PR 15093. Now I closed PR 15093. And resubmit code as PR 15095.

[animalize]

Search "Py_CHARMASK" in Python source code, there are more than a dozen Py_CHARMASK can be deleted:

cpython/Python/mystrtoul.c

Line 102 in e42b705

while (*str && Py_ISSPACE(Py_CHARMASK(*str)))

cpython/Python/dynload_aix.c

Line 143 in e42b705

while (Py_ISDIGIT(Py_CHARMASK(*message[i]))) message[i]++ ;

cpython/Modules/unicodedata.c

Line 963 in 530f506

h = (h * scale) + (unsigned char) Py_TOUPPER(Py_CHARMASK(s[i]));

...

[animalize]

Or remove Py_CHARMASK in Py_ISxxx/Py_TOLOWER/Py_TOUPPER macros?
Sometimes c is already a unsinged char, Py_CHARMASK is not necessary in these cases.

[qigangxu]

Thanks Ma,I also think all these redundant Py_CHARMASK should be deleted.

[qigangxu]

After the full search for the project code, there are more than a dozen similar issues that need to be modified. A new PR will be added later.

[animalize]

VC2017 optimizes multiple unsigned char)((c) & 0xff to a single movzx operation, maybe other compilers do it as well.
If so, there will be no performance changes.

[qigangxu]

Hi Ma,
I think this is a very valuable suggestion.
And if we want to delete the definition of Py_CHARMASK, maybe another issue needed.
I am not very sure about the considerations of this macro definition. And whether there are any side effects after deletion needs further discussion.

Thanks,
Best Regards

[shihai1991]

Due to this macro documented in https://docs.python.org/3.9/c-api/intro.html#c.Py_CHARMASK and we don't know how much user use this marco, I don't think we need delte this function(deleting operation should be careful).