change some statics to constants #25823

oli-obk · 2015-05-27T14:51:12Z

oli-obk · 2015-05-27T14:54:23Z

src/libstd/thread/local.rs

@@ -105,10 +105,12 @@ pub struct LocalKey<T> {
 #[allow_internal_unstable]
 macro_rules! thread_local {
    (static $name:ident: $t:ty = $init:expr) => (
+        #[cfg_attr(not(stage0), allow(static_could_be_const))]


these might have interior mutability... The error messages the user would get would be quite confusing, I'm not sure what to do. for now they are simply ignored

eddyb · 2015-05-27T16:01:51Z

At a quick glance, this doesn't seem to handle statics referencing other statics.
Everything else looks good.
The statics used by thread::LocalKey don't make much sense to me, they could just as well be consts.
cc @alexcrichton

oli-obk · 2015-05-27T16:37:49Z

The statics used by thread_local don't make much sense to me, they could just as well be consts.

won't calling the with function on LocalKey end up creating a new constant, applying with, and forgetting the value right after?

eddyb · 2015-05-27T17:00:11Z

@oli-obk sure, but the location of the struct is unimportant, the function pointer inside returns the actual TLS pointer.

alexcrichton · 2015-05-27T18:02:58Z

I don't think that there's a great way to draw a line and say precisely whether a value should be a constant or a static, so I would personally not be in favor of this lint (unless it's possibly allow-by-default). For example:

Many statics converted to constants here are quite large, and this can incur quite a bit of metadata bloat which is not always desired.
There's a legitimate use case for wanting a static to indeed have a significant address, for example the thread locals (which need the thread_local attribute)

There are certainly some switches from static to const in this PR which I think are fine, but I'm very worried about the noise a lint like this will generate.

eddyb · 2015-05-27T18:25:07Z

@alexcrichton Is there any case where you have a significant address and no inner mutability?
An immutable thread_local seems quite useless unless you just want the significant address (what for?).

The metadata bloat is more interesting. Reusing allocation space for constants across crates is something we should do before we switch over large constants to it.
Maybe this should also wait on a lint for large copies.

As for the thread_local question, I was referring to the structure holding the two function pointers which represent accessors for the initializer and the actual thread-local data, respectively.
I see no reason for keeping them in a static and I'm even wondering whether the accessors should be #[inner] to allow cross-crate devirtualization.

alexcrichton · 2015-05-27T18:45:12Z

Is there any case where you have a significant address and no inner mutability?

I don't personally have an example offhand, but I would not be prepared to say that 100% of these cases should be constants.

An immutable thread_local seems quite useless unless you just want the significant address (what for?).

We've had use-cases in the past for just having a significant address, that's how the old TLS system worked.

I see no reason for keeping them in a static and I'm even wondering whether the accessors should be #[inner] to allow cross-crate devirtualization.

Ideally I'd prefer to not have these weird layers of indirection, so in the future it may be possible to have a static, and I'm not sure if it's backwards compatible to switch from a const to a static, an interesting question though!

What's the #[inner] attribute you're mentioning?

oli-obk · 2015-05-27T19:02:53Z

src/librustc/lint/mod.rs

@@ -88,10 +88,14 @@ macro_rules! lint_initializer {
 macro_rules! declare_lint {
    // FIXME(#14660): deduplicate
    (pub $name:ident, $level:ident, $desc:expr) => (
+        // cannot be const, otherwise random lints fail to lint
+        #[cfg_attr(not(stage0), allow(static_could_be_const))]
        pub static $name: &'static ::rustc::lint::Lint


this is one example where it has to stay static

eddyb · 2015-05-27T19:04:43Z

Oops, that was meant to be #[inline]. Had "inner" stuck in my mind from all the TLS internals.

I definitely agree with keeping the door open for a cleaner design - I am guessing it's a matter of disallowing passing &'static LocalKey between threads (or not having it be 'static).
But isn't that backwards-incompatible?
Right now I don't see what prevents sharing or sending &MY_TLS_KEY across threads, as it's a simple pair of accessors.

This ability should also not depend on whether #[thread_local] isn't being used (on the platforms where it's not supported).
Is impl !Sync for LocalKey enough? Should it be done ASAP?

oli-obk · 2015-05-27T19:05:21Z

Many statics converted to constants here are quite large, and this can incur quite a bit of metadata bloat which is not always desired.

Won't the metadata be about the size of the saved static allocation space?

Also, many static arrays are not converted to constant arrays, but to &'static [T; N]. Which as far as I know simply creates the static anonymously.

eddyb · 2015-05-27T19:06:39Z

@oli-obk a static containing an array won't be duplicated across all crates using it. We should implement this as an optimization for consts (via available_externally).

oli-obk · 2015-05-27T19:08:54Z

so all pub const with a certain size get turned into statics?

eddyb · 2015-05-27T19:13:33Z

src/libstd/thread/local.rs

        static $name: ::std::thread::LocalKey<$t> = {
            #[cfg_attr(all(any(target_os = "macos", target_os = "linux"),
                           not(target_arch = "aarch64")),
                       thread_local)]
+            #[cfg_attr(not(stage0), allow(static_could_be_const))]


Wait, these ones shouldn't need the allow, AFAIK they always contain interior mutability.

then we have a bug on our hands

eddyb · 2015-05-27T19:42:54Z

@oli-obk no, all constants (no matter what size) should reuse the LLVM globals across crates, instead of duplicating them.
To still allow optimizations, it's possible to keep the current inlining behavior and add available_externally attributes to the globals, after ensuring that their symbol is present in some upstream crate.

alexcrichton · 2015-05-27T22:54:11Z

@eddyb

I definitely agree with keeping the door open for a cleaner design - I am guessing it's a matter of disallowing passing &'static LocalKey between threads (or not having it be 'static).

I wasn't specifically thinking about sending between threads, but yes this change may need to be made, I'd have to think about it.

@oli-obk

Won't the metadata be about the size of the saved static allocation space?

Unfortunately the metadata required to encode a static can often be orders of magnitude larger than that of the compiled version. For example a 1000 entry constant creates a 68K rlib while a static creates an 11K rlib.

@eddyb

We should implement this as an optimization for consts (via available_externally).

Note that we do not emit globals for constants, only if you take the address of them. We do already make use of available_externally where possible I believe as well.

eddyb · 2015-05-27T23:20:34Z

@alexcrichton if you copy a const value (e.g. a large array) to the stack, it will copy from .rodata, instead of generating code to write the data on the stack.
This applies for any constant rvalue.
One specific exception is [x; N], as it's always more efficient to initialize in-place (via memset or a custom loop). than to copy it (via memcpy or a custom loop - doing more work than in the memset case).

oli-obk · 2015-05-28T08:41:29Z

Unfortunately the metadata required to encode a static can often be orders of magnitude larger than that of the compiled version. For example a 1000 entry constant creates a 68K rlib while a static creates an 11K rlib.

I guess there's potential for optimization right there? But the duplication in every crate using it issue might then still be there.

bors · 2015-06-01T22:56:05Z

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

oli-obk · 2015-06-03T11:15:18Z

rebased and changed the lint to only warn for non-public items or public ref items. I also made sure that no large constants are publicly available due to my changes.

alexcrichton · 2015-06-05T16:36:28Z

src/libcore/macros.rs

@@ -15,7 +15,7 @@ macro_rules! panic {
        panic!("explicit panic")
    );
    ($msg:expr) => ({
-        static _MSG_FILE_LINE: (&'static str, &'static str, u32) = ($msg, file!(), line!());
+        const _MSG_FILE_LINE: (&'static str, &'static str, u32) = ($msg, file!(), line!());


This is a case where the use of a static is actually done as an optimization. This guarantees that the data here is translated at most once and makes calling the underlying function efficient. (e.g. this should remain a static)

(same below as well)

alexcrichton · 2015-06-05T16:39:30Z

I'm personally still worried about the number of false positives cropping up here, there's quite a few instances that are intended to be statics but are being changed to constants. Lints like this which may in theory hurt performance in various aspects I think should never need an opt-out of the warning, but instead the warning should only happen when it's 100% certainly an improvement.

oli-obk · 2015-06-06T20:14:36Z

yea. I agree, the false positive rate is insane. The programmer is already opting into a fixed address with static, adding another layer of lint-ignoring attributes is not the solution. I don't think there is a global optimum, therefor I removed the lint and just left the (to my eyes) obvious cases where a static should be a lint. Mainly non-public statics, small statics and (very few) obvious cases for constants

alexcrichton · 2015-06-07T17:17:28Z

src/librustc_unicode/u_str.rs

@@ -369,7 +369,7 @@ impl<'a> DoubleEndedIterator for Graphemes<'a> {
 }

 // https://tools.ietf.org/html/rfc3629
-static UTF8_CHAR_WIDTH: [u8; 256] = [
+const UTF8_CHAR_WIDTH: [u8; 256] = [


This and the ascii maps below are getting somewhat large, how come they're constants instead of statics?

they are not public, therefore I assumed they don't appear in the metadata

Constants can indirectly be required to go into the metadata, for example if they're used in an #[inline] function. If this function is never inlined then I don't believe there's a benefit one way or the other for using a static or a const, so it seems more appropriate to use a static as it's an assertion that this should never go into the metadata.

oli-obk · 2015-06-07T17:50:22Z

addressed comments and squashed

alexcrichton · 2015-06-08T03:49:45Z

@bors: r+ ec078a0

Thanks!

bors · 2015-06-08T04:55:00Z

⌛ Testing commit ec078a0 with merge bea1c4a...

@eddyb

r? @eddyb

bors · 2015-06-08T06:02:18Z

💔 Test failed - auto-mac-32-opt

oli-obk · 2015-06-08T06:11:50Z

I'm not really sure how I could have caused this failure, I didn't change any docs...

alexcrichton · 2015-06-08T06:31:24Z

@bors: retry

On Sun, Jun 7, 2015 at 11:11 PM, Oliver Schneider [email protected]
wrote:

I'm not really sure how I could have caused this failure, I didn't change
any docs...

—
Reply to this email directly or view it on GitHub
#25823 (comment).

bors · 2015-06-08T07:35:00Z

⚡ Previous build results for auto-mac-64-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt...

bors · 2015-06-08T09:54:31Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt

rust-highfive assigned eddyb May 27, 2015

oli-obk reviewed May 27, 2015
View reviewed changes

eddyb reviewed May 27, 2015
View reviewed changes

oli-obk force-pushed the static_to_const_lint branch 2 times, most recently from d2aad59 to 935e641 Compare June 3, 2015 11:11

alexcrichton reviewed Jun 5, 2015
View reviewed changes

oli-obk force-pushed the static_to_const_lint branch from 935e641 to 54c7372 Compare June 6, 2015 20:10

oli-obk changed the title ~~lint on statics that could be replaced by constants~~ change some statics to constants Jun 6, 2015

oli-obk force-pushed the static_to_const_lint branch from 54c7372 to fa99d90 Compare June 7, 2015 08:45

alexcrichton reviewed Jun 7, 2015
View reviewed changes

oli-obk force-pushed the static_to_const_lint branch 2 times, most recently from d1eae73 to ec078a0 Compare June 7, 2015 17:49

change some statics to constants

ec078a0

bors added a commit that referenced this pull request Jun 8, 2015

Auto merge of #25823 - oli-obk:static_to_const_lint, r=alexcrichton

bea1c4a

r? @eddyb

bors merged commit ec078a0 into rust-lang:master Jun 8, 2015

oli-obk deleted the static_to_const_lint branch July 3, 2015 07:48

change some statics to constants #25823

change some statics to constants #25823

Conversation

oli-obk commented May 27, 2015

Choose a reason for hiding this comment

eddyb commented May 27, 2015

oli-obk commented May 27, 2015

eddyb commented May 27, 2015

alexcrichton commented May 27, 2015

eddyb commented May 27, 2015

alexcrichton commented May 27, 2015

Choose a reason for hiding this comment

eddyb commented May 27, 2015

oli-obk commented May 27, 2015

eddyb commented May 27, 2015

oli-obk commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented May 27, 2015

alexcrichton commented May 27, 2015

eddyb commented May 27, 2015

oli-obk commented May 28, 2015

bors commented Jun 1, 2015

oli-obk commented Jun 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jun 5, 2015

oli-obk commented Jun 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jun 7, 2015

alexcrichton commented Jun 8, 2015

bors commented Jun 8, 2015

bors commented Jun 8, 2015

oli-obk commented Jun 8, 2015

alexcrichton commented Jun 8, 2015

bors commented Jun 8, 2015

bors commented Jun 8, 2015