Skip to content

Improve compression of TastyString #5307

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

Merged

Conversation

nicolasstucki
Copy link
Contributor

Improves #3877

'{ { val y = ~x * ~x; ~powerCode(n / 2, '(y)) } }
was reduced from 206 bytes to 172 bytes

'{ ~x * ~powerCode(n - 1, x) }
was reduced from 143 bytes to 128 bytes

Improves scala#3877

`'{ { val y = ~x * ~x; ~powerCode(n / 2, '(y)) } }`
was reduced from 206 bytes to 172 bytes

`'{ ~x * ~powerCode(n - 1, x) }`
was reduced from 143 bytes to 128 bytes
@nicolasstucki nicolasstucki force-pushed the improve-compression-of-tasty-string branch from 48b1452 to a3a41bb Compare October 22, 2018 12:46
Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

Should we bump the TASTY version?

i += 1
def pickle(bytes: Array[Byte]): Pickled = {
val str = new String(Base64.getEncoder().encode(bytes), UTF_8)
def split(sliceEnd: Int, acc: List[String]): List[String] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a reimplementation of sliding? If so why not str.sliding(maxStringSize, maxStringSize).toList?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This version is more efficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal opinion is to go with the simplest solution unless it is performance sensitive and it happen to be a hotspot. No strong feelings though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it to the sliding. We can always change it later if needed.

@nicolasstucki
Copy link
Contributor Author

This does not affect the tasty format. We do not need to update the version number.

@odersky
Copy link
Contributor

odersky commented Oct 22, 2018

I don't know what the policy wrt bumbing the Tasty version is. Say you have two PR's and they each bump the version from 11 to 12. Then what is the right version? Alternatively, could we bump the TASTY version only for each release if there is a change in TASTY? That would limit the bumping speed to at most one every 6 weeks or so. I am not sure what scheme is preferable. What do others think?

@nicolasstucki
Copy link
Contributor Author

Regardless of the Tasty version bumping scheme, this PR does not need a tasty version bump. Maybe I should add a version to the scheme used to serialize tasty bytes into strings.

Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

LGTM

i += 1
def pickle(bytes: Array[Byte]): Pickled = {
val str = new String(Base64.getEncoder().encode(bytes), UTF_8)
def split(sliceEnd: Int, acc: List[String]): List[String] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

My personal opinion is to go with the simplest solution unless it is performance sensitive and it happen to be a hotspot. No strong feelings though

@nicolasstucki nicolasstucki merged commit 2167869 into scala:master Oct 23, 2018
@allanrenucci allanrenucci deleted the improve-compression-of-tasty-string branch October 23, 2018 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants