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

Convert extra::ebml to extra::atom_trees #9303

Closed
brson opened this issue Sep 19, 2013 · 21 comments
Closed

Convert extra::ebml to extra::atom_trees #9303

brson opened this issue Sep 19, 2013 · 21 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@brson
Copy link
Contributor

brson commented Sep 19, 2013

This module doesn't actually implement ebml correctly and only exists to support rustc's metadata. Make a few changes to optimize it:

  • Rename it from ebml to atom_trees, change all internal naming
  • Make tags an atom (fourcc tag) of native endianness + 32-bit length with native endianness. This should be faster than the variable-length tags ebml uses.

cc #9255

@emberian
Copy link
Member

Please also use little endian, not big endian.

@emberian
Copy link
Member

(Oh, it seems that was mentioned; nevermind)

@michaelwoerister
Copy link
Member

What's an atom tree? I can't find anything with that name on the internet.

@emberian
Copy link
Member

cc @pcwalton (I recall him talking about this).

I think it's just whatever ebml is now, though... not really sure.

bors added a commit that referenced this issue Feb 9, 2014
I was looking into #9303 and was curious if this would still be valuable. @kballard had already done 99% of the work, so I brought the branch up to date and added a feature gate. Any feedback would be appreciated; I wasn't sure if this should be set up as a syntax extension with `#[macro_registrar]`, and if so, where it should be located.

Original PR is here: #9255

TODO:
* [x] Convert to loadable syntax extension
* [x] Default to big endian
* [x] Add `target` identifier
* [x] Expand to include code points 128-255
@ahmedcharles
Copy link
Contributor

Is this done or still being worked on?

@huonw
Copy link
Member

huonw commented May 26, 2014

Neither, if you wish to take it, it's all yours.

@sfackler
Copy link
Member

Would it make sense to pull this into librustc? I can't imagine anyone else wanting to use it.

@ahmedcharles
Copy link
Contributor

Should this still be called atom_trees? I wouldn't mind working on this, but it would require some guidance, if anyone's interested.

@brson
Copy link
Contributor Author

brson commented May 28, 2014

@sfackler I'd rather not, just because build times, but we are growing some crates that just exist to support rustc, like graphviz.

@ahmedcharles Yes, I think atom_trees is fine. A good first step would just be to do the rename, then worry about optimizing the representation.

@pcwalton May have opinions.

@ahmedcharles
Copy link
Contributor

It seems someone renamed this to rbml. Is this issue still relevant?

@emberian
Copy link
Member

@ahmedcharles the optimizations are, the name isn't.

@druzn3k
Copy link

druzn3k commented Dec 21, 2014

I'm willing to close this bug; is anyone still working on this?

@flaper87
Copy link
Contributor

@druzn3k I'd assume no one is. Feel free to ping me if you have questions.

@flaper87 flaper87 added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Dec 21, 2014
@druzn3k
Copy link

druzn3k commented Dec 21, 2014

Thank you @flaper87 :)

@flaper87
Copy link
Contributor

@druzn3k actually, This bug has been partially fixed. We don't have an ebml module anymore but a rbml crate as of e1dcbef

I believe the endianness change is yet to be done

@flaper87
Copy link
Contributor

@erickt IIRC, you moved ebml into rbml, mind chiming in on what's left to do in this issue? 🍰

@bagedevimo
Copy link

I'd like to get into rust dev, is someone willing to mentor me on in endianness change?

@druzn3k
Copy link

druzn3k commented Jan 20, 2015

Hi @bagedevimo, i was working on the issue, but i cannot find the time to continue do to personal issues. It's all yours! 👍

@steveklabnik
Copy link
Member

removing a-libs because we don't expose this publicly

bors added a commit that referenced this issue Mar 3, 2015
This is a series of individual but correlated changes to the metadata format. The changes are significant enough that it (finally) bumps the metadata encoding version. In brief, they altogether reduce the total size of stage1 binaries by 27% (!!!!). Almost every low-hanging fruit has been considered and fixed; see the individual commits for details.

Detailed library (not just metadata) size changes for x86_64-unknown-linux-gnu stage1 binaries (baseline being 3a96d6a):

````
   before     after  delta path
--------- --------- ------ --------------------------------
  1706146   1050412  38.4% liballoc-4e7c5e5c.rlib
   398576    152454  61.8% libarena-4e7c5e5c.rlib
    71441     56892  20.4% libarena-4e7c5e5c.so
 14424754   5084102  64.8% libcollections-4e7c5e5c.rlib
 39143186  14743118  62.3% libcore-4e7c5e5c.rlib
   195574    188150   3.8% libflate-4e7c5e5c.rlib
   153123    152603   0.3% libflate-4e7c5e5c.so
   477152    215262  54.9% libfmt_macros-4e7c5e5c.rlib
    77728     66601  14.3% libfmt_macros-4e7c5e5c.so
  1216936    684104  43.8% libgetopts-4e7c5e5c.rlib
   207846    181116  12.9% libgetopts-4e7c5e5c.so
   349722    147530  57.8% libgraphviz-4e7c5e5c.rlib
    60196     49197  18.3% libgraphviz-4e7c5e5c.so
   729842    259906  64.4% liblibc-4e7c5e5c.rlib
   349358    247014  29.3% liblog-4e7c5e5c.rlib
    88878     83163   6.4% liblog-4e7c5e5c.so
  1968508    732840  62.8% librand-4e7c5e5c.rlib
  1968204    696326  64.6% librbml-4e7c5e5c.rlib
   283207    206589  27.1% librbml-4e7c5e5c.so
 72369394  46401230  35.9% librustc-4e7c5e5c.rlib
 11941372  10498483  12.1% librustc-4e7c5e5c.so
  2717894   1983272  27.0% librustc_back-4e7c5e5c.rlib
   501900    464176   7.5% librustc_back-4e7c5e5c.so
    15058     12588  16.4% librustc_bitflags-4e7c5e5c.rlib
  4008268   2961912  26.1% librustc_borrowck-4e7c5e5c.rlib
   837550    785633   6.2% librustc_borrowck-4e7c5e5c.so
  6473348   6095470   5.8% librustc_driver-4e7c5e5c.rlib
  1448785   1433945   1.0% librustc_driver-4e7c5e5c.so
 95483688  94779704   0.7% librustc_llvm-4e7c5e5c.rlib
 43516815  43487809   0.1% librustc_llvm-4e7c5e5c.so
   938140    817236  12.9% librustc_privacy-4e7c5e5c.rlib
   182653    176563   3.3% librustc_privacy-4e7c5e5c.so
  4390288   3543284  19.3% librustc_resolve-4e7c5e5c.rlib
   872981    831824   4.7% librustc_resolve-4e7c5e5c.so
 1817642  14795426  18.6% librustc_trans-4e7c5e5c.rlib
  3657354   3480026   4.8% librustc_trans-4e7c5e5c.so
 16815076  13868862  17.5% librustc_typeck-4e7c5e5c.rlib
  3274439   3123898   4.6% librustc_typeck-4e7c5e5c.so
 21372308  14890582  30.3% librustdoc-4e7c5e5c.rlib
  4501971   4172202   7.3% librustdoc-4e7c5e5c.so
  8055028   2951044  63.4% libserialize-4e7c5e5c.rlib
   958101    710016  25.9% libserialize-4e7c5e5c.so
 30810208  15160648  50.8% libstd-4e7c5e5c.rlib
  6819003   5967485  12.5% libstd-4e7c5e5c.so
 58850950  31949594  45.7% libsyntax-4e7c5e5c.rlib
  9060154   7882423  13.0% libsyntax-4e7c5e5c.so
  1474310   1062102  28.0% libterm-4e7c5e5c.rlib
   345577    323952   6.3% libterm-4e7c5e5c.so
  2827854   1643056  41.9% libtest-4e7c5e5c.rlib
   517811    452519  12.6% libtest-4e7c5e5c.so
  2274106   1761240  22.6% libunicode-4e7c5e5c.rlib
--------- --------- ------ --------------------------------
499359187 363465583  27.2% total
````

Some notes:

* Uncompressed metadata compacts very well. It is less visible for compressed metadata but still it achieves about 5~10% reduction.
* *Every* commit is designed to reduce the metadata in one way. There is absolutely no negative impact associated to changes (that's why the table above doesn't contain a minus delta).
* I've confirmed that this compiles through `make all`, making it almost correct. Other platforms have to be tested though.
* Oh, I'll rebase this as soon as I have spare time, but I guess this needs an extensive review anyway.
* I haven't rigorously checked the encoder and decoder performance. I tried to minimize the impact (some encodings are actually simpler than the original), but I'm not sure.

Fixes #2743, #9303 (partially) and #21482.
@arielb1
Copy link
Contributor

arielb1 commented Sep 27, 2015

Using fixed-length instead of variable-length tags would lead to significant bloat. I don't think endianness costs that much.

@steveklabnik
Copy link
Member

closing based on #21482 (comment)

flip1995 pushed a commit to flip1995/rust that referenced this issue Aug 11, 2022
Fix ICE when reading literals with weird proc-macro spans

fixes rust-lang#9297
changelog: Fix ICE when reading literals with weird proc-macro spans
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

No branches or pull requests