GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Reduce branches in PyUnicode_MAX_CHAR_VALUE by kumaraditya303 · Pull Request #92142 · python/cpython

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented May 2, 2022

Copy link
Copy Markdown
Contributor

Offers a minor 5.5 % speed improvement in microbenchmark but less branches are better especially when it can be done with a small lookup table.

Benchmark:

./python -m timeit -s "a='🚀'" "a*1000"
# Before
2000000 loops, best of 5: 182 nsec per loop
# After
2000000 loops, best of 5: 172 nsec per loop

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kumaraditya303
reduce branches
ca2513a
@bedevere-bot bedevere-bot added the awaiting review label May 2, 2022
@kumaraditya303 kumaraditya303 self-assigned this May 2, 2022
@kumaraditya303
kumaraditya303 requested a review from gvanrossum May 2, 2022 14:31
@kumaraditya303
kumaraditya303 force-pushed the fast-string branch from 134c043 to ca2513a Compare May 2, 2022 14:31
@kumaraditya303 kumaraditya303 added skip issue skip news labels May 2, 2022
@kumaraditya303

kumaraditya303 commented May 2, 2022

Copy link
Copy Markdown
Contributor Author

I don't understand why msvc is failing to compile.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303
kumaraditya303 force-pushed the fast-string branch 2 times, most recently from 504bde2 to ca2513a Compare May 2, 2022 15:51
@kumaraditya303

kumaraditya303 commented May 2, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

MSVC is failing to compile and test_cppext is failing as C++ does not support designated initializers in array by #91321
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303 kumaraditya303 mentioned this pull request May 2, 2022
[C API] Test that the Python C API is compatible with C++ #91321
Closed
gvanrossum
gvanrossum reviewed May 2, 2022
View reviewed changes
Comment thread Include/cpython/unicodeobject.h Outdated
}

static const Py_UCS4 max_char_values[] = {
[PyUnicode_1BYTE_KIND] = 0xffU,

@gvanrossum gvanrossum May 2, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

So just do it the oldfashioned way?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kumaraditya303 kumaraditya303 May 2, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done but I wanted to use designated initializer, it looks better

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
kumaraditya303 added 2 commits May 2, 2022 16:07
@kumaraditya303
old style
fa06bb0
@kumaraditya303
const
2782355
@kumaraditya303
kumaraditya303 requested a review from gvanrossum May 3, 2022 04:09
gvanrossum
gvanrossum reviewed May 3, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Did you benchmark it on a build with PGO and LTO? Could you also compare before/after for 1-byte wide and 2-byte wide strings? On my Mac (in debug mode, so far) the odd thing is that the 2-byte mode is faster than the 1-byte mode.

I also wonder whether having static data inside a static inline function (that's potentially expanded many times) is a good idea.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/cpython/unicodeobject.h
0xffffU, /* PyUnicode_2BYTE_KIND */
0,
0x10ffffU, /* PyUnicode_4BYTE_KIND */
};

@gvanrossum gvanrossum May 3, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Since kind is 3 bits, maybe add extra zero entries for a total of 8?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gvanrossum

gvanrossum commented May 3, 2022

Copy link
Copy Markdown
Member

I do think it's odd that it fails on MSVC, since designated initializers in arrays are supported -- see e.g. Include/internal/pycore_opcode.h. I wonder if MSVC doesn't like having it in a static inline function?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303

kumaraditya303 commented May 4, 2022

Copy link
Copy Markdown
Contributor Author

Well the benchmark is so small that pgo and lto gives inconsistent result because of cpu boost and binary layout. I reject this as this is insignificant and not worth it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303 kumaraditya303 closed this May 4, 2022
@kumaraditya303
kumaraditya303 deleted the fast-string branch May 4, 2022 10:40
@gvanrossum

gvanrossum commented May 4, 2022

Copy link
Copy Markdown
Member

Okay, we learned a few things. :-)

  • Use PGO+LTO for benchmarking, even micro-benchmarking.
  • Benchmarking is important.
  • Maybe a static array inside a static inline function isn't a great idea.
  • MSVC seems picky about those.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303

kumaraditya303 commented May 4, 2022

Copy link
Copy Markdown
Contributor Author

Use PGO+LTO for benchmarking, even micro-benchmarking.

Yes, but lto takes a lot of time to compile and can vary in results up to 5 to 10%.

Maybe a static array inside a static inline function isn't a great idea.

Linker would de-dedup those in the executable.

MSVC seems picky about those.

MSVC is picky about designated initializers in array in C++ mode, in C mode it works fine.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented May 4, 2022

Copy link
Copy Markdown
Member

Got it!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@kumaraditya303 kumaraditya303

Labels

awaiting review skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@kumaraditya303 @gvanrossum @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.