Quick Rust Improvement Ideas — Language & Std (Community Brainstorm)

These are at odds. One reason for large target folders is the incremental cache to make dev builds faster. Or if you mean smaller binaries, that takes optimization work which is also in opposition to fast compile speed.

"It should be faster and smaller" is an obvious wish -- everyone would happily have that -- but it's not really a helpful idea, IMHO.

3 Likes

I think making a warning on an instance of HashMap with the default hasher would be too disruptive, even if it's somehow limited to only one instance per build. Making this warning would require everyone who cares about having no warnings to disposition every HashMap in their codebase and either change the hasher if it only handles trusted inputs or annotate an #[expect(the_new_lint, reason = "..")] attribute if it handles untrusted inputs.

I maintain a moderate-sized codebase where a significant fraction is no_std code that can't use heap-allocated memory. But the parts that are allowed heap allocation have 55 different HashMaps and 38 HashSets (same weakness). If your warning was added, we'd suddenly have to look at nearly a hundred different data structures and perform this analysis to decide how to handle it.

It might work to make a category of clippy lints for very performance-sensitive code and put this lint in that category, so people who care about optimization can turn it on for the hot parts of their code.

Yeah, I understand it's for caching build artifacts, my point is why does it get so large when other languages like Zig don't have this problem, even though Zig also has incremental builds. No, I'm not talking about binary size, there's no way binary size alone could reach tens of gigabytes for a medium to large project where the actual user written code is still minimal, say a backend with a framework + database + JSON + Tokio and fewer than 5 API endpoints. I'm talking about the build artifacts themselves, as I already described the problem above that is the artifacts in the target folder

I think you might be overstating the disruption here. Even if you have 100 HashMaps, it would still only produce a single warning, no per instance line details, if you want line level information, you can run with --expand-warn <warning-key>. If anything, that's exactly what makes it effective, seeing the warning prompts you to pause and ask yourself "does any of my HashMaps actually receive data from an external source?" If yes, that's an opportunity to optimize. If not, or if you've determined optimization isn't needed, you can simply ignore it. Warnings are designed to be ignorable, just like any other warning

As for putting it in Clippy, I don't think that fully solves the problem. In practice, most people don't run Clippy regularly, it's an extra step that many users simply won't bother. Surfacing this as a built in warning reaches all users by default, which is precisely the point

I would like to see better support from standard library for converting data structures like arrays and new types between different views with identical memory representation.

Safe transmute project would have helped, but seems to have stalled and maybe there is an opportunity to do better even before that.

I had written a lot of unsafe boilerplate over the years and extracted two crates recently to isolate unsafe:

  • array-reshape: zero-cost and safe conversions between things like [[T; N]; M] and [T; N * M] with compile-time checks (as opposed to checked or unchecked APIs currently available in the standard library); supports stable Rust (including const fn) and const Trait on nightly
  • transparent-wrapper: zero-cost and safe conversions between new type and its inner representation through a simple derived trait; supports stable Rust (including const fn) and const Trait on nightly

These are generally straightforward conversions, but may still have sneaky pitfalls. Having standard library support for these would have been really nice.

2 Likes

I currently think that the std library should provide, s!(...), a macro for expediting the creation of a String. The current approach isn't ergonomic.

Granted, this not strictly a language issue but a library issue.

How often do you actually create Strings from a literal, without formatting? In my experience such occurrences are very rare.

But, the DoS-resistant default hashing function was presumably chosen for a good reason, right?

If the problem this tries to solve is "people using the wrong hashmap for their use-case because they didn't read the docs", how will this prevent those non-docs-reading people from not reading the docs and blindly doing what the warning suggests, i.e. changing all their HashMap uses with a FashHash, to get rid of the warning? Because, if they do, we end up with the same exact problem of people using the wrong hashmap for their use-case because they didn't read the docs, except that now the wrong hashmap may cause a security problem instead of a performance problem. Plus, legit uses also get an annoying warning.

I do agree with the default HashMap hasher not being a good choice for many (maybe most) cases. And if the Rust project considers both the cryptographically-secure hasher and the fast hasher as equally-valid use-cases, then maybe the right thing to do would be to expose the two on equal footing: std::collections::SecureHashMap (a rename of the current HashMap), and std::collections::FastHashMap. Though i think i'd prefer not renaming anything, and maybe just having an alternative fast hasher on the stdlib and let users swap that in if they need it.

3 Likes

Yes, so if you build your web service/program in Rust. It doesn't get HashDos-ed by default. And honestly it's a sane default.

A HashDos is basically attacker figuring out there is a HashMap, and using specially crafted insert request to turn your O(1) gets into O(n).

The proposal doesn't say the default DoS resistant algorithm is bad. The warning is designed to be informative, not alarmist. The idea is that it communicates something like: "This HashMap uses SipHash by default, which is designed to prevent hash flooding from external input. If your HashMap doesn't process external input, consider switching to std::hash::FastHash for better performance. [Read more link]"

With that context in the warning itself, the "blindly following" concern largely goes away. A developer reading it would naturally ask themselves "do I handle external input here?", check their code, and make an informed decision. Nobody consciously trades away security once they understand why SipHash is there and what external input means in practice, the warning makes that explicit in concrete operational terms, not just abstract "prevents DoS"

Not renaming, renaming would break existing code and isn't the right path. The proposal is purely additive as I already wrote above introduce std::hash::FastHash (the name can be anything but still communicates that it is fast), and surface a compiler warning to guide users toward making a conscious choice. Existing HashMap behavior stays untouched

It's time to make the Rust compiler one step further by guiding users to write high performance code, not stopping at memory safety. The Rust ecosystem has a lot of suboptimal code out there, unnecessary heap allocations, excessive RefCell usage, and yes, HashMap with SipHash where it doesn't need to be

1 Like

This also sounds related to type piracy in Julia, which is discouraged even if possible, because it can cause bugs easily Clarification on type piracy - General Usage - Julia Programming Language

I am always opposed to "suppress this if you read the docs" warnings, especially in rustc itself. Don't annoy the people who did read them just because some people didn't.

(This is in the same category as super-annoying "did you know that == on floats might not be what you wanted?" warnings. If you don't know what every computer scientist should know, that's great to link from the docs, but it's not the compiler's problem.)

6 Likes

How would it be annoying when it's actually reminding you of something that genuinely happens in practice? The reality is that a huge number of codebases use the default HashMap for purely internal data where SipHash's hash flooding protection provides zero benefit but costs real performance. A warning grounded in what actually happens in the wild is more useful than silently assuming every HashMap needs DoS protection

And rustc already has plenty of "suppress this if you know what you're doing" warnings that nobody calls annoying, using non upper case name for global variables, using non camel case name for typed, non snake case, single use lifetimes, trivial casts, elided lifetimes in paths, and more. All of these fire on perfectly valid, compiling code, and nudge you toward a better practice. Nobody argues those should be removed because they annoy the people who already know the conventions

Why would this be any different? If anything, this warning has better justification than a naming convention lint, it has a direct performance impact. It is clearly would be useful in practice, given that in the reality many codebases use the default HashMap purely for internal input data processing

If you have a compiler that doesn't just teach you how to write memory safe code, but also teaches you how to write high performance code, that will also raise the quality of Rust code out there

The massive difference is if there's something that's always better.

It's not "suppress the lint if your transmute is correct"; it's "u32::from_ne_bytes is literally always better than using a transmute for that, so just change it".

1 Like

And i'd expect that the vast majority of programs are not so performance-sensitive, and that changing the hashing function of their hashmaps wouldn't make any significant difference. And i also think that for the relatively few projects that reach a point where optimizing this would make a difference, the path to discovering this issue with HashMap and remediating it is pretty straightforward.

In other words, i think the projects that would benefit from this proposed warning are a relatively small minority, while the warning would negatively affect the majority (all?) of HashMap users.

But this is just my intuition really, which is mostly based on anecdotal experience and relatively little performance optimization work. I don't have any real statistical numbers on this.

When you say "the reality is", does that mean you have some actual statistics on this? If so, i'd love to see them, and hopefully recalibrate my intuition.

3 Likes

Similarly, switching away from the default hashing algorithm when its extra features aren't needed is always the better choice when processing internally sourced input

And I did mention many other warnings above, like naming conventions,why was that skipped? And here's the full list, there are even more there : Warn-by-default Lints - The rustc book

If you have to say "when", then it's not "always".

3 Likes

Rust as a fundamental component is to provide tools that are secure and as zero cost as possible, and to communicate that well so that if there is a cost, that cost is very easy to know about, that's what good design looks like. Rust should not make assumptions about what performance the user will make, that's not Rust's business, Rust's focus is only to provide the best as possible and communicate it clearly and easily, let the user choose based on their use case after knowing from the information

No, assuming the user will profile their application to find the overhead is very unrealistic from a language perspective, overhead can come from small accumulations that pile up when it becomes 1 big app but originally each was small and frequent. HashMap is a fundamental data type, if you often read Rust code in the wild, objectively you can say 98% of code does not change the default hasher algorithm when processing input that comes from internal. They might not know that the HashMap they use to process internal input is much slower than C++ unordered_map, also slower than Go map, slower than Zig map. Even the Rust compiler itself says it's slow and doesn't use it. In 1 project, HashMap is used over and over and over again, not just once, because it's a fundamental component. 1 project uses 10 libraries, those libraries could also use HashMap a lot, because that's the std component for O(1) operations, what if the libraries used by those libraries also use HashMap? A fundamental component very easily scales to being used many times over. Even popular libraries still use the default hasher even though it's clearly processing data from internal, this is what was said earlier about reading documentation being enough, but in reality it's not

Tokio uses default hasher even though if you read the code there it's clearly internal data not from user input:

Rust Analyzer, read it here:

Regex, please read the code, it processes internal input: https://github.com/search?q=repo%3Arust-lang%2Fregex+hashmap&type=code

Polars, many actually process internal data: https://github.com/search?q=repo%3Apola-rs%2Fpolars+hashmap&type=code

Lighthouse, many process internal data too: https://github.com/search?q=repo%3Asigp%2Flighthouse+hashmap&type=code

Diesel, the popular SQL library, unfortunately uses the slow default hasher to process data that is clearly isolated and not user input: https://github.com/search?q=repo%3Adiesel-rs%2Fdiesel+hashmap&type=code

SQLx, another popular SQL library other than Diesel suffers from the same unoptimized code: https://github.com/search?q=repo%3Atransact-rs%2Fsqlx+hashmap&type=code

Oxigraph, a database library, contains the same unnecessary overhead: https://github.com/search?q=repo%3Aoxigraph%2Foxigraph+hashmap&type=code

And many much more. I could say with very high confidence that 98% of all Rust code in the world does not change the default hasher even though the data being processed is internal or isolated and not user input. Because it is very very very rare that I see anyone modify the hash algorithm. They assume it's already faster than HashMap in other languages, when in fact it's not

It is always better in the domain of processing internal data. And why haven't you addressed the naming convention warnings I mentioned, and many more, here's the full list, there are many there that if weighed are actually and objectively this performance warning weighs more than them: Warn-by-default Lints - The rustc book