Skip to content

Conversation

@ericsnowcurrently
Copy link
Member

@ericsnowcurrentlyericsnowcurrently commented Sep 16, 2021

The result on debug vs. non-debug builds were slightly different. This PR fixes that by making an additional pass over the object to ensure "refs" are used more than once. Consequently, marshal.dumps() is substantially slower. To mitigate this, I've added to it a keyword-only arg, "stable", to allow folks to opt out.

FYI, I discovered gh-8293 after the fact, which turns out to have a lot of similarities to this PR.

@ericsnowcurrently
Copy link
MemberAuthor

@gvanrossum, is something like this what you had in mind?

Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

Probably, but I'm probably not going to be able to review it before Monday.

@ericsnowcurrentlyericsnowcurrently removed the request for review from tiranSeptember 16, 2021 22:06
@ericsnowcurrentlyericsnowcurrently changed the title bpo-45186: Produce consistent output from marshal.dumps().bpo-34093: Produce consistent output from marshal.dumps().Sep 16, 2021
Indicates the data format that dumps should use.
/
*
stable: bool = True
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

It would probably make sense to default to False for now, so users would effectively opt-in to the performance penalty. (The same goes for marshal.dump().)

@ericsnowcurrently
Copy link
MemberAuthor

ericsnowcurrently commented Sep 20, 2021

As I noted in [bpo-34093](https://bugs.python.org/issue34093#msg402244), we could take a different approach to get the same stable output without having to call w_object() twice. However, this PR would probably be worth landing in the meantime.

@ericsnowcurrently
Copy link
MemberAuthor

This PR should probably also add a flag to determine if the import machinery would use marshal.dumps(code, stable=True). The same goes for the compileall module.

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsbot added the stale Stale PR or inactive for long period of time. label Oct 21, 2021
@gvanrossum
Copy link
Member

gvanrossum commented Oct 21, 2021 via email

@methane
Copy link
Member

We need to this to support "reproducible build" in downstream.

@github-actionsgithub-actionsbot removed the stale Stale PR or inactive for long period of time. label Oct 22, 2021
Copy link
Member

@iritkatrieliritkatriel left a comment

Choose a reason for hiding this comment

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

This has merge conflicts now.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@gvanrossumgvanrossum changed the title bpo-34093: Produce consistent output from marshal.dumps().gh-78274: Produce consistent output from marshal.dumps().Nov 29, 2022
@encukou
Copy link
Member

Closing; see discussion in the issue.

@encukouencukou closed this Mar 26, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@ericsnowcurrently@gvanrossum@methane@bedevere-bot@encukou@iritkatriel@the-knights-who-say-ni@ezio-melotti