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

Make libsyntax compile with minimal amount of feature flags #24518

Closed
13 of 18 tasks
erickt opened this issue Apr 17, 2015 · 7 comments
Closed
13 of 18 tasks

Make libsyntax compile with minimal amount of feature flags #24518

erickt opened this issue Apr 17, 2015 · 7 comments

Comments

@erickt
Copy link
Contributor

erickt commented Apr 17, 2015

This a meta-bug that's tracking getting libsyntax to compile with the smallest possible feature flag set so that I can use it as the foundation for syntex. I've done a bunch of the work in #24487, but here are some more longer term dependencies that need some planning:

Here's what needs to be done:

#[feature(core)]:

  • Int::zero() in ast
  • ToPrimitive in codemap
  • usize::to_u32() in codemap
  • ptr::read_and_drop() in fold
  • slice::ref_slice() in parser

#[feature(collections)]:

  • std::collections::BitSet
  • String::escape_default

#[feature(libc)]:

  • libc::c_uint in codemap.rs
  • libc::isatty(libc::STDERR_FILENO) in diagnostic.rs

#[feature(unicode)]:

  • char.width() in diagnostic.rs
  • char.is_xid_start() in lexer
  • char.is_xid_continue() in lexer

#[feature(path_ext)]:

  • Path::exists() - easily replaced with call to metadata

#[feature(str_char)]:

  • str::char_at() - replace with s.chars().take()
  • str::slice_shift_char()

#[feature(rustc_private)]:

@erickt
Copy link
Contributor Author

erickt commented Apr 22, 2015

@alexcrichton: Regarding ptr::read_and_drop, the primary user of this code is syntax::ptr::P::map, which uses it to avoid an alloc when folding over ast. I could copy that functionality into libsyntax at the cost of having to copy some unstable drop code as well, which seems slightly risky. I could also reimplement map to just make a new allocation, at the cost of it being ~40% slower. Do you think this is on a critical-enough path that copying ptr::read_and_drop() is worth it? I'd be happy to compare the compile times of rust with and without the extra alloc to see if there's any real impact.

@erickt
Copy link
Contributor Author

erickt commented Apr 22, 2015

@alexcrichton: Interestingly enough, I just added another variation that modified P::map into:

    pub fn map3<F>(mut self, f: F) -> P<T> where
        F: FnOnce(&mut T),
    {
        f(&mut *self.ptr);
        self
    }

And it's ~66% faster. So maybe I should just replace all the users with that pattern. It's unfortunately a little less convenient, but it might not be that bad if most of the users are isolated to syntax::fold.

edit: here's the benchmark.

@alexcrichton
Copy link
Member

@erickt if we could get by with the &mut T variant I think that would be best. The current implementation is not panic-safe into the future as eventually there will be no drop flag and calling read_and_drop followed by an arbitrary closure will not be sound (e.g. a panic in the closure will drop invalid data).

You can also just replace the map3 function with foo(&mut ptr) due to deref coercions, so if you're able to finagle it by just removing usage of the map function itself that may also be best.

@erickt
Copy link
Contributor Author

erickt commented Apr 24, 2015

@alexcrichton: Regarding BitSet, it's only used in attr.rs in order to speed up tracking if an attribute has been used or not. We got three options for syntex:

  1. Replace it in libsyntax with HashSet with the assumption that we don't actually have that many attributes, that the overhead of using a whole usize might not really impact memory usage.
  2. Pull BitSet library into an external crate and use it in syntex.
  3. Have a patch in syntex that swaps attr.rs's use of BitSet with HashSet.

What do you think is best?

@alexcrichton
Copy link
Member

@erickt in general I'm hesitant to contort libsyntax too much, so I would optimize for the least invasive solution. I'm not sure what the stability track is for BitSet, as that may be a plausible course of action.

bors added a commit that referenced this issue Jun 10, 2015
Gets libsyntax one step closer to running on stable (see #24518).
Closes #24757, erickt's previous attempt at this.
bors added a commit that referenced this issue Jun 10, 2015
Gets libsyntax one step closer to running on stable (see #24518).
Closes #24757, erickt's previous attempt at this.
@jonas-schievink
Copy link
Contributor

Why is this tagged A-metadata?

@Mark-Simulacrum Mark-Simulacrum removed the A-metadata Area: Crate metadata label May 12, 2017
@Mark-Simulacrum
Copy link
Member

Closing in favor of (newer) #41732.

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

No branches or pull requests

5 participants