Skip to content
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

Human readable formatting for numbers #6314

Merged

Conversation

straight-shoota
Copy link
Member

This PR adds three methods for formatting numbers in human-readable formats:

  • Number#format: Similar to #to_s but allows customized separator and delimiter (useful for localization) and specifying the number of printed decimal places.
  • Number#humanize: Formats the most significant digits of the number with a prefix indicating the magnitude. Output parameters, prefixes etc. can be fully customized.
  • Int#humanize_bytes: Specialization of Number#humanize for binary units
123.4567.format(decimal_places: 3) # => "123.457"
1_200_000_000.humanize # => "1.2G"
1536.humanize_bytes # => "1.5KB"

Number#humanize is already put to use in the benchmark module.

src/number.cr Outdated Show resolved Hide resolved
src/int.cr Outdated Show resolved Hide resolved
src/int.cr Outdated Show resolved Hide resolved
src/int.cr Outdated Show resolved Hide resolved
src/number.cr Outdated Show resolved Hide resolved
src/number.cr Outdated Show resolved Hide resolved
src/number.cr Outdated Show resolved Hide resolved
@asterite
Copy link
Member

asterite commented Jul 3, 2018

Can this be in a number/humanize require? The std keeps growing and base compile times are getting slower and slower.

@RX14
Copy link
Contributor

RX14 commented Jul 3, 2018

@asterite methods dont affect compile times at all unless they are used. This method is unused unless you use the benchmark harness then you're doing a release build anyway and you don't care.

@asterite
Copy link
Member

asterite commented Jul 3, 2018

@RX14 Almost: the parser still has to parse it and build an AST for it. Every bit counts.

@straight-shoota
Copy link
Member Author

@asterite I don't think that extra tiny bit is worth the nuisance to have to require number/humanize. I'd like to encourage people to use these formatter methods for human-readable output and hope they'll be used in many Crystal applications 👍

@wooster0
Copy link
Contributor

wooster0 commented Jul 3, 2018

The prelude.cr/std should be shrinked generally I think. Every single time you compile some crystal file, all the methods in every require in prelude.cr get parsed. Even if you only use 3 or no methods of all these thousand methods.

time(crystal emptyfile.cr --prelude=empty); time(crystal emptyfile.cr)
real    0m0.349s
user    0m0.156s
sys     0m0.094s

real    0m2.157s
user    0m1.594s
sys     0m1.219s

@straight-shoota
Copy link
Member Author

@r00ster91 this is not the place to discuss prelude.cr.

@asterite
Copy link
Member

asterite commented Jul 3, 2018

@straight-shoota Could you use descriptive commit messages for fixups? Just having fixup! with no context makes it really hard to review commits.

@straight-shoota
Copy link
Member Author

@asterite The first line of the commit message needs to reference the targeted commit, otherwise !fixup doesn't work. Descriptions are only visible in the second line.
Maybe I'm missing something but I can't find a way to prevent that.

@asterite
Copy link
Member

asterite commented Jul 3, 2018

Is fixup! a convention or something?

@yxhuvud
Copy link
Contributor

yxhuvud commented Jul 3, 2018

@asterite if you use git rebase --autosquash, then it is magic that automatically make git rebase do the right thing.

@straight-shoota You can refer to refspecs instead. see https://robots.thoughtbot.com/autosquashing-git-commits

@straight-shoota
Copy link
Member Author

@yxhuvud That still doesn't allow custom descriptions in the first line of the commit message, does it?

@yxhuvud
Copy link
Contributor

yxhuvud commented Jul 3, 2018

@straight-shoota Ah, no. That is a pity.

@RX14
Copy link
Contributor

RX14 commented Jul 3, 2018

@asterite it's not just a convention, it's an official part of git. git commit --fixup=sha1 and then git commit --autosquash master.

@RX14
Copy link
Contributor

RX14 commented Jul 3, 2018

@asterite judging by --stats, parsing uses less than 1% of the time in any program.

I'm entirely unconcerned by parse time. By the time parse time is the compiler bottleneck, we will have won.

@asterite
Copy link
Member

asterite commented Jul 3, 2018

That's because parse stats just show the time for parsing the main file. When a require is processed, that parsing time happens in top-level, I think.

At one point all parsing was done in parsing, but since that has changed the parsing step has very little use now as stats info.

Regardless, it's not just parsing time, it's holding the AST nodes in memory for the entire compilation.

But yeah, let's not care. Let's wait for the magical solution that will enable incremental compilation :-)

@RX14
Copy link
Contributor

RX14 commented Jul 3, 2018

@asterite Parse and top level together for the compiler use 473 milliseconds out of 28.618 seconds. That's 1.65%. It's still not a bottleneck. For cached builds, that increases to about 2.5% of time.

I'm not saying compiler performance isn't useful, but this is picking the wrong thing to worry about.

@asterite
Copy link
Member

asterite commented Jul 3, 2018

Sounds good.

src/int.cr Outdated Show resolved Hide resolved
Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

I think this is good, but I still think the IEC standard should be the default.

Another place where it would be possible to use this method is in HTTP::LogHandler.

@straight-shoota
Copy link
Member Author

What do others think about IEC vs JEDEC? (see thread #6314 (comment))

@Blacksmoke16
Copy link
Member

Will this make it into 0.27.1?

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

I would prefer the number and string code to be in a humanize.cr file in the std lib (even if it's included in the prelude). That would help to keep related code together.

@bcardiff
Copy link
Member

@straight-shoota what do you think about organizing all the humanize code in a separate file/folder?

@RX14
Copy link
Contributor

RX14 commented Jan 26, 2019

@bcardiff he's on holiday...

@bcardiff bcardiff modified the milestones: 0.27.1, 0.28.0 Jan 30, 2019
@straight-shoota
Copy link
Member Author

@bcardiff done

@Sija
Copy link
Contributor

Sija commented Feb 5, 2019

Hmm, seems that lately CI bugs increased in frequency.

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

The build error should go out after a rebase. But I would merge this with squash though. So GTG on my side.

The code looks better in the current layout. Thanks, @straight-shoota.

@straight-shoota straight-shoota force-pushed the jm/feature/humanize branch 2 times, most recently from 76f2ded to 778579e Compare February 7, 2019 16:25
@straight-shoota
Copy link
Member Author

CI is still broken @bcardiff

@bcardiff
Copy link
Member

bcardiff commented Feb 7, 2019

hmm, it's the same issue as in #7179.

Lot's of cc: error: ****.o: No such file or directory

@asterite
Copy link
Member

asterite commented Feb 7, 2019

Can those failing builds be reproduced? Are they using docker? If we can't reproduce these locally it'll be very hard to figure out what has gone wrong. And I'm almost sure it has nothing to do with a specific PR, I'm almost sure it's something about the CI (disk, memory?)

@bcardiff
Copy link
Member

bcardiff commented Feb 7, 2019

The CRYSTAL_CACHE_DIR in the CI uses the /tmp inside docker. That was done a long time ago maybe there is something special about /tmp (a cleanup could be triggered?) although its writes are performed over the container as any other folder.

Let's see if using the default location helps at both PRs.


spoiler: it didn't work

@j8r
Copy link
Contributor

j8r commented Feb 7, 2019

Usually /tmp is mounted as tmps, which is RAM. If both the storage and the compilation use RAM, we can have issues if the RAM is full. I haven't found anything more.

@bcardiff
Copy link
Member

bcardiff commented Feb 8, 2019

It seems that the issue is not /tmp but somewhere when emitting the .o.tmp files the process stops. Leaving partial .o.tmp files and some missing .o oddly enough the missing files seem to be consistent.

Running in CircleCI make std_spec FLAGS=--no-debug seems to make things pass. Always.

I don't know if it is something about the debug information itself or the file size or something with the memory buffers.

That's all I could find.

@asterite
Copy link
Member

asterite commented Feb 8, 2019

I suggest trying to run a debug build if the compiler with debug output (puts, after each .o is generated, check if the file is there later, etc.). Otherwise I don't know how can we debug this.

* `Number#format`: Similar to `#to_s` but with more options for customizing the
  output format. Decimal separator and thousands delimiter can be specified
  (useful for localization) as well as the number of printed decimal places.
* `Number#humanize`: Pretty prints the number in a human-readable format.
  Formats the most significant digits of the number with a prefix indicating the
  magnitude. Output parameters, prefixes etc. can be fully customized.
* `Int#humanize_bytes`: Specialization of `Number#humanize` for binary units.

```cr
123.4567.format(decimal_places: 3) # => "123.457"
1_200_000_000.humanize # => "1.2G"
1536.humanize_bytes # => "1.5KiB"
```
@straight-shoota
Copy link
Member Author

Rebased and ready to ship.

@straight-shoota straight-shoota merged commit b0e88cf into crystal-lang:master Mar 12, 2019
@straight-shoota straight-shoota deleted the jm/feature/humanize branch March 12, 2019 23:16
urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Mar 20, 2019
* 'master' of github.com:crystal-lang/crystal:
  Change the font-weight used in Playground (crystal-lang#7552)
  Fix formatting absolute paths (crystal-lang#7560)
  Refactor IO::Syscall as IO::Evented (crystal-lang#7505)
  YAML: fix test checking serialization of slices for libyaml 0.2.2 (crystal-lang#7555)
  Let Array#sort only use `<=>`, and let `<=>` return `nil` for partial comparability (crystal-lang#6611)
  Update `to_s` and `inspect` to have similar signatures accross the stdlib (crystal-lang#7528)
  Fix restriction of valid type vs free vars (crystal-lang#7536)
  Rewrite macro spec without executing a shell command (crystal-lang#6962)
  Suggest `next` when trying to break from captured block  (crystal-lang#7406)
  Fix GenericClassType vs GenericClassInstanceType restrictions (crystal-lang#7537)
  Add human readable formatting for numbers (crystal-lang#6314)
  Add command and args to execvp error message (crystal-lang#7511)
  Implement Set#add? method (crystal-lang#7495)
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.