Skip to content

Using appliedParams instead of "supplied" params in compressBegin()#1982

Merged
bimbashrestha merged 2 commits intofacebook:devfrom
bimbashrestha:quick
Feb 4, 2020
Merged

Using appliedParams instead of "supplied" params in compressBegin()#1982
bimbashrestha merged 2 commits intofacebook:devfrom
bimbashrestha:quick

Conversation

@bimbashrestha
Copy link
Copy Markdown
Contributor

@bimbashrestha bimbashrestha commented Jan 31, 2020

EDIT:

This PR doesn't change compression behavior, it is necessary for enabling ldm with dictionaries because resetCCtx changes ldm params

@Cyan4973
Copy link
Copy Markdown
Contributor

Cyan4973 commented Feb 1, 2020

Using appliedParams instead of "supplied" params in compressBegin()

Is this what we want ?
Describe the difference, and why it's better.

@felixhandte
Copy link
Copy Markdown
Contributor

On further consideration, this is not a bug. If ZSTD_resetCCtx_internal() modifies cparams, then this is a bug that needs to be fixed. But it doesn't, and shouldn't.

@Cyan4973, there is no difference. But I guess the point is that if there were, it would be important for us to use the applied params, not the requested params.

@bimbashrestha
Copy link
Copy Markdown
Contributor Author

bimbashrestha commented Feb 1, 2020

On further consideration, this is not a bug. If ZSTD_resetCCtx_internal() modifies cparams, then this is a bug that needs to be fixed. But it doesn't, and shouldn't.

It calls ZSTD_ldm_adjustParameters when we're in ldm mode though (which sets the ldm params). And I need to be able to use that inside loadDictionaryContent when filling the ldm hash tables.

@felixhandte
Copy link
Copy Markdown
Contributor

Ok, sorry, yes. I was talking specifically about the cparams.

@terrelln
Copy link
Copy Markdown
Contributor

terrelln commented Feb 1, 2020

While not a bug, it is necessary to fill LDM tables while loading the dictionary.

@bimbashrestha bimbashrestha changed the title [bug] Using appliedParams instead of "supplied" params in compressBegin() Using appliedParams instead of "supplied" params in compressBegin() Feb 3, 2020
@bimbashrestha bimbashrestha merged commit 0cbafe3 into facebook:dev Feb 4, 2020
Sign up for free to 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.

5 participants