Introduce FeatureValue type to represent features table values #5270

djc · 2018-03-31T17:53:03Z

This is the next step towards #1286 (after #5258). The goal here is to have a central place in the code where feature strings are interpreted as (a) a feature, (b) a dependency or (c) a dependency/feature combo, and anchor that interpretation in the type system as an enum.

I've spent quite a bit of effort avoiding extra string allocations, complicating the code a bit; notice in particular the use of Cow<str> in FeatureValue variants, and the slight workaround in Context::resolve_features() and build_requirements(). I hope this is all okay.

cc @Eh2406

Eh2406 · 2018-04-02T01:42:43Z

I am very looking forward to reviewing this. From the description it sounds like a good approach. With the holiday I have not yet had a chance to review it.

Eh2406 · 2018-04-02T17:43:01Z

I took a look at it and I think it looks correct. I think using InternedString would make the code easier to read then using Cow. I have tried to convert feature string to be interned before and the hardest part was the code you are rewriting now. I have wondered before what percentage of ram assigned to cargo is storing copies of the string "serde" :-P

djc · 2018-04-03T18:00:26Z

@Eh2406 I've done another take using InternedString in FeatureValue instead of Cow<str>:

https://github.com/rust-lang/cargo/compare/master...djc:interned-feature-reqs?expand=1

You're right that FeatureValue itself becomes easier to read and more compact. Do we need to run any benchmarks to determine the effect of these changes, or can we just move forward with the InternedString version of my branch?

Eh2406 · 2018-04-03T18:27:18Z

That does look clearer! I'd be ok just moving forward, the only part that can be slow is InternedString::new and .to_string(). We can always use more InternedString's so that neither of those are in the resolver loop. We could do that now or later, for the resolver it is definitely Performance Third Priority.

alexcrichton · 2018-04-03T18:51:15Z

@bors: delegate=Eh2406

bors · 2018-04-03T18:51:16Z

✌️ @Eh2406 can now approve this pull request

djc · 2018-04-03T19:43:18Z

Okay, I've pushed the InternedString-based branch into this PR. Note that this still calls InternedString::new() a number of times in Context::resolve_features(), which is part of the call chain in resolver::activate(). Not sure if that's part of the hot loop, though.

Eh2406 · 2018-04-03T19:57:21Z

Unfortunately it is in the hot loop, but let's get it correct and clear before we think about performance. What do you see as the next step? Do you want that step to be in this PR, or in a separate one? Do you want all things that are storing feature strings to get converted to use FeatureValue?

bors · 2018-04-03T23:12:13Z

☔ The latest upstream changes (presumably #5287) made this pull request unmergeable. Please resolve the merge conflicts.

djc · 2018-04-04T07:27:59Z

I'd prefer to land this and do next steps in the next PR. The next step will be to introduce the namespaced-features flag in the package section, which will change how package manifest feature values get interpreted. It's probably useful if you look over the design in #1286, in particular #1286 (comment).

What other areas of the code do you think are working with feature strings? The thing about FeatureValue is that it can only interpret feature strings in the context of a particular Summary, so some of the places feature strings get used (including the whole call chain from command-line arguments into the resolver) don't make sense to convert because they're not as specific to a particular Summary.

Eh2406

2 little nits.

Eh2406 · 2018-04-04T15:32:04Z

src/cargo/core/interning.rs

+    }
+}
+
+unsafe impl Send for InternedString {}


I think this is a rebase error, it was recently removedin https://github.com/rust-lang/cargo/pull/5287/files#diff-fc6e9b7a46297a8f6d7851cce9978683L94 as a &'static str is already send and sync.

Eh2406 · 2018-04-04T15:32:12Z

src/cargo/core/summary.rs

+/// * A feature in a depedency
+///
+/// The selection between these 3 things happens as part of the construction of the FeatureValue.
+/// It stores a `Cow<str>`, so both `String` and `&str` can be used; typically, the former will


"It stores a Cow<str>" this is a little out of date

Eh2406 · 2018-04-04T15:43:54Z

Edit: Just notes that you had already changed a bunch of places to use FeatureValue that I was going to suggest you change. Sorry for the confusion.

I will happily r+ when the nits are addressed. I wanted to ask, instead of guessing, as I have seen this go wrong in both directions. I have seen PR's merged while I was still adding the rest of the big picture, and I have seen PR's waiting for approval because the PR was only for the first step.

Thank you for adding abstraction to some of the most "stringly typed" parts of the resolver. It was desperately needed.

djc · 2018-04-04T16:04:35Z

Nits fixed, thanks for the review!

Eh2406 · 2018-04-04T16:16:06Z

@bors r+

bors · 2018-04-04T16:16:07Z

📌 Commit 7621108 has been approved by Eh2406

bors · 2018-04-04T16:16:15Z

⌛ Testing commit 7621108 with merge 61787c7a6733d5bf3dfd84c4865d0ca5d1f1b04b...

bors · 2018-04-04T16:26:06Z

💔 Test failed - status-travis

Eh2406 · 2018-04-04T16:28:31Z

@bors retry

bors · 2018-04-04T16:28:38Z

⌛ Testing commit 7621108 with merge 75f77fc...

@Eh2406

Introduce FeatureValue type to represent features table values This is the next step towards #1286 (after #5258). The goal here is to have a central place in the code where feature strings are interpreted as (a) a feature, (b) a dependency or (c) a dependency/feature combo, and anchor that interpretation in the type system as an enum. I've spent quite a bit of effort avoiding extra string allocations, complicating the code a bit; notice in particular the use of `Cow<str>` in `FeatureValue` variants, and the slight workaround in `Context::resolve_features()` and `build_requirements()`. I hope this is all okay. cc @Eh2406

bors · 2018-04-04T17:16:07Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Eh2406
Pushing 75f77fc to master...

Eh2406 · 2018-04-04T17:32:46Z

tests/testsuite/metadata.rs

-                    "default_feat"
+                    {
+                       "Feature": "default_feat"
+                    }
                  ],


I only just notest that this change, after the merge. I don't know why/if this change in structures is ok. Will this break things using metadata?

Yeah, this definitely will break consumers that look at the features field. Could we preserve the old format here?

Oops -- sorry, I thought that was only used for testing. Will follow up with a fix.

@matklad

Revert serialization of features to string type Accidentally broken during #5270 and only noticed after merge. cc @matklad @Eh2406

@djc

Intern more strings As pointed out in #5270 (comment), that clean up adds the mildly expensive `InternedString::new` to the hot path in the resolver. The process of this PR is: 1. Find a `InternedString::new` in the hot path. 2. replace the argument of type `&str` that is passed along to `InternedString::new` with the type `InternedString` 3. add an `InternedString::new` to the callers until it type checked. 4. Repeat from step 1. This stop if: - It was traced back to something that was already an `InternedString` - It was traced back to a `.to_string()` call - It was in a persistent object creation cc: - @djc this is building on your work, I don't want to get in your way. - @alexcrichton is this making things clearer and do you want to see a performance check?

@djc

use InternedString's ability to be a &'static str to appease the borrow checker This "slight workaround" was added in rust-lang#5270. So @djc dose this still look correct?

@djc

simplify build_requirements use InternedString's ability to be a &'static str to appease the borrow checker This "slight workaround" was added in #5270. So @djc does this still look correct and an improvement?

rust-highfive assigned alexcrichton Mar 31, 2018

djc force-pushed the feature-requirements branch from b6b5958 to f09576b Compare March 31, 2018 20:32

djc force-pushed the feature-requirements branch from f09576b to 4ba56f0 Compare April 3, 2018 19:38

Define FeatureMap type as an abstraction

116a3cd

djc force-pushed the feature-requirements branch from 4ba56f0 to 8ed9299 Compare April 4, 2018 07:23

Eh2406 requested changes Apr 4, 2018

View reviewed changes

djc added 2 commits April 4, 2018 18:03

Introduce FeatureValue enum for tracking feature types

7b54226

Extract function to build feature map for readability

7621108

djc force-pushed the feature-requirements branch from 8ed9299 to 7621108 Compare April 4, 2018 16:03

bors merged commit 7621108 into rust-lang:master Apr 4, 2018

Eh2406 reviewed Apr 4, 2018

View reviewed changes

djc mentioned this pull request Apr 4, 2018

Revert serialization of features to string type #5292

Merged

Eh2406 mentioned this pull request Apr 4, 2018

Intern more strings #5293

Merged

bors added a commit that referenced this pull request Apr 4, 2018

Auto merge of #5292 - djc:feature-requirements, r=alexcrichton

0729e06

Revert serialization of features to string type Accidentally broken during #5270 and only noticed after merge. cc @matklad @Eh2406

Eh2406 added a commit to Eh2406/cargo that referenced this pull request May 10, 2018

simplify build_requirements

ea053e3

use InternedString's ability to be a &'static str to appease the borrow checker This "slight workaround" was added in rust-lang#5270. So @djc dose this still look correct?

Eh2406 mentioned this pull request May 10, 2018

simplify build_requirements #5512

Merged

ehuss added this to the 1.27.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce FeatureValue type to represent features table values #5270

Introduce FeatureValue type to represent features table values #5270

djc commented Mar 31, 2018

Eh2406 commented Apr 2, 2018

Eh2406 commented Apr 2, 2018

djc commented Apr 3, 2018

Eh2406 commented Apr 3, 2018

alexcrichton commented Apr 3, 2018

bors commented Apr 3, 2018

djc commented Apr 3, 2018

Eh2406 commented Apr 3, 2018

bors commented Apr 3, 2018

djc commented Apr 4, 2018 •

edited

Loading

Eh2406 left a comment

Eh2406 Apr 4, 2018

Eh2406 Apr 4, 2018

Eh2406 commented Apr 4, 2018

djc commented Apr 4, 2018

Eh2406 commented Apr 4, 2018

bors commented Apr 4, 2018

bors commented Apr 4, 2018

bors commented Apr 4, 2018

Eh2406 commented Apr 4, 2018

bors commented Apr 4, 2018

bors commented Apr 4, 2018

Eh2406 Apr 4, 2018

matklad Apr 4, 2018

djc Apr 4, 2018

Introduce FeatureValue type to represent features table values #5270

Introduce FeatureValue type to represent features table values #5270

Conversation

djc commented Mar 31, 2018

Eh2406 commented Apr 2, 2018

Eh2406 commented Apr 2, 2018

djc commented Apr 3, 2018

Eh2406 commented Apr 3, 2018

alexcrichton commented Apr 3, 2018

bors commented Apr 3, 2018

djc commented Apr 3, 2018

Eh2406 commented Apr 3, 2018

bors commented Apr 3, 2018

djc commented Apr 4, 2018 • edited Loading

Eh2406 left a comment

Choose a reason for hiding this comment

Eh2406 Apr 4, 2018

Choose a reason for hiding this comment

Eh2406 Apr 4, 2018

Choose a reason for hiding this comment

Eh2406 commented Apr 4, 2018

djc commented Apr 4, 2018

Eh2406 commented Apr 4, 2018

bors commented Apr 4, 2018

bors commented Apr 4, 2018

bors commented Apr 4, 2018

Eh2406 commented Apr 4, 2018

bors commented Apr 4, 2018

bors commented Apr 4, 2018

Eh2406 Apr 4, 2018

Choose a reason for hiding this comment

matklad Apr 4, 2018

Choose a reason for hiding this comment

djc Apr 4, 2018

Choose a reason for hiding this comment

djc commented Apr 4, 2018 •

edited

Loading