Closed
Conversation
Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Closed
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Closed
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
This was referenced Sep 22, 2019
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
dzhulgakov
reviewed
Sep 25, 2019
Collaborator
dzhulgakov
left a comment
There was a problem hiding this comment.
This is fancy :) Is there a measurable impact for compile time? (I'm a bit scared of too many crc compiler computation)
Contributor
Author
|
I did not measure but template instantiation rules should make sure that it's only computed once per type. The C++14 computation is an iterative loop and should be very fast, crc is designed to be fast and is a single pass over the data. The C++11 crc computation is recursive, so it might in theory be a little bit slower, but probably also negligible. Also, it's tail recursive, so the compiler might even be able to optimize it into an iterative loop. |
Contributor
|
Please measure the compile time impact before and after. You can do it on a single stressful file, if you like. |
Contributor
|
What happens if the crc collides? |
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Contributor
Author
|
@ezyang if crc collides, we will give the same id to two types, which would be an issue. But it's crc64. This is very unlikely to collide, there is some more info here. If you think collisions are too likely, I could add a runtime mechanism to check for them (i.e. a runtime type registry that remembers type names and corresponding ids), but I think that's probably not worth it as long as we only need it for collision checking. I don't expect any compile time issues, but I'll do some measurements. |
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Contributor
Author
|
I did some compile time measurements. Setup
Summary
Without this PRAverage user build time over both runs: 210m19s With this PRAverage user build time over both runs: 210m42s |
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
smessmer
added a commit
that referenced
this pull request
Oct 14, 2019
Pull Request resolved: #26502 Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of `__PRETTY_FUNCTION__` in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. ghstack-source-id: 91872668 Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/)
ezyang
reviewed
Oct 14, 2019
|
|
||
| template <typename T> | ||
| inline C10_HOST_CONSTEXPR uint64_t type_index_impl() noexcept { | ||
| // Idea: __PRETTY_FUNCTION__ (or __FUNCSIG__ on msvc) contains a qualified name |
Contributor
There was a problem hiding this comment.
I'm not sure how I feel about this lol
Contributor
Author
There was a problem hiding this comment.
It's more reliable, portable and easier to understand/change than the ODR hacks we had before.
Contributor
|
I'm not sure how I feel about this... I guess I'd feel better about this if we had some way (even if it's a runtime, test only check) to see if there any conflicts for any of the type ids we use inside our application... |
Contributor
Author
|
@ezyang there's tests in c10/test/util/TypeIndex_test.cpp. They show there aren't conflicts for a few common types. |
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of __PRETTY_FUNCTION__ in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. Differential Revision: [D17488861](https://our.internmc.facebook.com/intern/diff/D17488861/) [ghstack-poisoned]
ezyang
approved these changes
Oct 15, 2019
Contributor
|
This pull request has been merged in 6f865c1. |
thiagocrepaldi
pushed a commit
to thiagocrepaldi/pytorch
that referenced
this pull request
Feb 4, 2020
Summary: Pull Request resolved: pytorch#26502 Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of `__PRETTY_FUNCTION__` in a constexpr context. However, since GCC5 this is possible and we can use this trick. This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately. ghstack-source-id: 91896920 Test Plan: unit tests Differential Revision: D17488861 fbshipit-source-id: ce7b059d7c8686b69cb091a4a8beaf4b96391343
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
Create type ids at compile time instead of incrementing a counter at runtime. This is done by computing a compile time crc64 on the type name. We couldn't do this before, because we still used GCC4 and that compiler didn't support the use of PRETTY_FUNCTION in a constexpr context. However, since GCC5 this is possible and we can use this trick.
This does not change the semantics of preallocated type ids. I actually think we don't need to preallocate anymore, but I split the removal of preallocation into a separate diff to be able to test it separately.
Differential Revision: D17488861