Checkpoint to phase 2 of the great ustringhash conversion #1612

lgritz · 2022-10-20T03:48:29Z

Reminder: phase 1 changed the RendererServices APIs to be based completely on ustringhash for public method calls, not ustring.

This PR starts along the path of phase 2.

In oslconfig.h, I define a new type ustringrep which is the in-memory/in-shader CPU-side representation of a ustring. Currently, it's still equivalent to a ustring.

I also define a type called ustring_pod, which is the "plain old data" representation inside a ustringrep (i.e., a const char* when it's a ustring, and a uint64_t when it's a ustringhash). For reasons not important, it's exceedingly hard to pass a true ustring/ustringhash as parameters to or from LLVM-jitted code without running afoul of LLVM's type checking, so it's just easier to pass the underlying data type.

So the main work of phase 2 is to change everything related to shader data from a ustring into a ustringrep, and things passed as const char* into passing a ustring_pod. In some sense, it's just renaming and type conversion plumbing. When that's done, phase 3 is to throw the switch and change the definition of ustringrep to ustringhash (and of course fix everything that's broken).

This PR does not complete phase 2, but takes a big bite out of it. It's an arbitrary bite, not much rhyme or reason to which specific parts I did so far. Just working on it piece by piece and felt that this was a place where much has been done, everything still works, and it's a reviewable chunk without getting too much out of control.

Areas I worked on are related to range_check, raytype, getattribute, bind_interpolated_param, uninit_check, naninf_check, getmessage/setmessage, some texture setup, pointcloud.

Signed-off-by: Larry Gritz lg@larrygritz.com

AlexMWells · 2022-10-20T17:15:55Z

src/liboslexec/llvm_instance.cpp

@@ -1294,8 +1294,13 @@ BackendLLVM::initialize_llvm_group()
        TypeSpec rettype = OSLCompilerImpl::type_from_code(types, &advance);
        types += advance;
        std::vector<llvm::Type*> params;
+        // bool debug = OIIO::Strutil::contains(funcname, "range");


Did you mean to leave these commented out lines here?

no, that's a mistake, will remove

AlexMWells · 2022-10-20T17:20:04Z

src/liboslexec/opmessage.cpp

 {
-    const ustring& name(USTR(name_));
-    const ustring& sourcefile(USTR(sourcefile_));
+    ustringhash name       = ustringhash_from(USTR(name_));


Does USTR(...) casting need to be involved anymore? I thought a benefit of these changes is to not need that very pointer/reinterpret_cast'ish and just use the ustring_rep or ustring_pod?

I think in most cases, that's where we'll eventually get to, yes.

AlexMWells · 2022-10-20T17:24:30Z

src/liboslexec/oslexec_pvt.h

+USTR(ustring_pod s) noexcept
+{
+    return OSL::bitcast<ustringrep>(s);
+}


Should USTR(...) be removed and just use USTREP because their implementations are identical. If not, maybe a comment here about the transition plan.

I think I'm leaning that way as I touch things. Mostly I was trying USTREP on for size because USTR was making me continually wonder whether it was a ustring or a ustringrep.
Will transition this over time.

AlexMWells · 2022-10-20T17:41:33Z

Consider changing name of ustring_pod to ustring_param as that makes it more clear where its going to be used.

AlexMWells · 2022-10-20T17:42:35Z

When all this is totally done (past stage 3 with the switch thrown), would the idea be to remove ustringrep and just have ustringhash?

AlexMWells · 2022-10-20T17:45:56Z

src/include/OSL/oslconfig.h.in

@@ -181,6 +214,18 @@ ustringrep_from(ustring u)
 #endif
 }

+/// Convenience function to convert to a ustringrep.
+inline ustringrep
+ustringrep_from(string_view s)


should additional overloads of
ustringrep ustringrep_from(ustring_pod)
be provided and used instead of the USTREP(...) or USTR(...) casts as it better represents the transition?

lgritz · 2022-11-03T21:37:36Z

Consider changing name of ustring_pod to ustring_param as that makes it more clear where its going to be used.

Ha, it was ustring_param until the last minute, when I thought that was unclear and renamed it ustring_pod. I'm still on the fence about the final name. I think where my head was can be summarized as that "param" made me think "how is this different from passing a ustring as the param?" whereas the pod was to emphasize that it's the non-class, plain-old-data representation of what's inside a ustring.

lgritz · 2022-11-03T21:38:38Z

When all this is totally done (past stage 3 with the switch thrown), would the idea be to remove ustringrep and just have ustringhash?

Yeah, I think so. I wanted a temporary name in the interim that I can easy switch back and forth. Once everything is working, it may just get wholesale replaced by ustringhash.

lgritz · 2022-11-03T21:39:35Z

I think we should treat all this nomenclature as temporary. When this work gets completed, some of them will no longer be necessary at all, and for others we can then choose what names we want on a permanent basis.

AlexMWells · 2022-11-03T21:57:27Z

it was ustring_param until the last minute, when I thought that was unclear and renamed it ustring_pod. I'm still on the fence about the final name. I think where my head was can be summarized as that "param" made me think "how is this different from passing a ustring as the param?" whereas the pod was to emphasize that it's the non-class, plain-old-data representation of what's inside a ustring.

I had similar issue, mostly when do I use ustring_pod vs. ustring vs ustringhash

AlexMWells · 2022-11-03T22:12:55Z

I think it just might be a bit weird because its in an intermediate state. Forward looking,
ustringrep would just be gone, replaced with ustringhash
ustring_pod would be gone, replaced with ustringhash::hash_t
?

lgritz · 2022-11-03T22:14:24Z

Yeah, I think something like that is the final state.

AlexMWells · 2022-11-03T22:16:30Z

Maybe don't worry about the names then (as they are intermediate) but I do recommend killing off the casting USTR macros and using helper functions.

Reminder: phase 1 changed the RendererServices APIs to be based completely on ustringhash for public method calls, not ustring. This PR starts along the path of phase 2. In oslconfig.h, I define a new type `ustringrep` which is the in-memory/in-shader CPU-side representation of a ustring. Currently, it's still equivalent to a ustring. I also define a type called `ustring_pod`, which is the "plain old data" representation inside a ustringrep (i.e., a const char* when it's a ustring, and a uint64_t when it's a ustringhash). For reasons not important, it's exceedingly hard to pass a true ustring/ustringhash as parameters to or from LLVM-jitted code without running afoul of LLVM's type checking, so it's just easier to pass the underlying data type. So the main work of phase 2 is to change everything related to shader data from a ustring into a ustringrep, and things passed as const char* into passing a ustring_pod. In some sense, it's just renaming and type conversion plumbing. When that's done, phase 3 is to throw the switch and change the definition of ustringrep to ustringhash (and of course fix everything that's broken). This PR does not complete phase 2, but takes a big bite out of it. It's an arbitrary bite, not much rhyme or reason to which specific parts I did so far. Just working on it piece by piece and felt that this was a place where much has been done, everything still works, and it's a reviewable chunk without getting too much out of control. Areas I worked on are related to range_check, raytype, getattribute, bind_interpolated_param, uninit_check, range_check, naninf_check, getmessage/setmessage, some texture setup, pointcloud. Signed-off-by: Larry Gritz <lg@larrygritz.com>

Signed-off-by: Larry Gritz <lg@larrygritz.com>

lgritz · 2022-11-04T18:54:04Z

Ok, I'm going to merge as it is and move on, cleaning up these things we've mentioned as I go.

AlexMWells · 2022-11-04T19:20:47Z

Sounds good to me

AlexMWells reviewed Oct 20, 2022

View reviewed changes

lgritz force-pushed the lg-ustringrep branch from 0ad8ea0 to 75fdb52 Compare November 3, 2022 22:15

lgritz added 2 commits November 4, 2022 00:50

review fixes

a9674d8

Signed-off-by: Larry Gritz <lg@larrygritz.com>

lgritz force-pushed the lg-ustringrep branch from 75fdb52 to a9674d8 Compare November 4, 2022 07:50

lgritz merged commit bd7fd4c into AcademySoftwareFoundation:main Nov 4, 2022

lgritz deleted the lg-ustringrep branch September 15, 2023 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checkpoint to phase 2 of the great ustringhash conversion #1612

Checkpoint to phase 2 of the great ustringhash conversion #1612

lgritz commented Oct 20, 2022

AlexMWells Oct 20, 2022

lgritz Nov 3, 2022

AlexMWells Oct 20, 2022

lgritz Nov 3, 2022

AlexMWells Oct 20, 2022

lgritz Nov 3, 2022

AlexMWells commented Oct 20, 2022 •

edited

AlexMWells commented Oct 20, 2022

AlexMWells Oct 20, 2022 •

edited

lgritz commented Nov 3, 2022

lgritz commented Nov 3, 2022

lgritz commented Nov 3, 2022

AlexMWells commented Nov 3, 2022

AlexMWells commented Nov 3, 2022 •

edited

lgritz commented Nov 3, 2022

AlexMWells commented Nov 3, 2022

lgritz commented Nov 4, 2022

AlexMWells commented Nov 4, 2022

Checkpoint to phase 2 of the great ustringhash conversion #1612

Checkpoint to phase 2 of the great ustringhash conversion #1612

Conversation

lgritz commented Oct 20, 2022

AlexMWells Oct 20, 2022

Choose a reason for hiding this comment

lgritz Nov 3, 2022

Choose a reason for hiding this comment

AlexMWells Oct 20, 2022

Choose a reason for hiding this comment

lgritz Nov 3, 2022

Choose a reason for hiding this comment

AlexMWells Oct 20, 2022

Choose a reason for hiding this comment

lgritz Nov 3, 2022

Choose a reason for hiding this comment

AlexMWells commented Oct 20, 2022 • edited

AlexMWells commented Oct 20, 2022

AlexMWells Oct 20, 2022 • edited

Choose a reason for hiding this comment

lgritz commented Nov 3, 2022

lgritz commented Nov 3, 2022

lgritz commented Nov 3, 2022

AlexMWells commented Nov 3, 2022

AlexMWells commented Nov 3, 2022 • edited

lgritz commented Nov 3, 2022

AlexMWells commented Nov 3, 2022

lgritz commented Nov 4, 2022

AlexMWells commented Nov 4, 2022

AlexMWells commented Oct 20, 2022 •

edited

AlexMWells Oct 20, 2022 •

edited

AlexMWells commented Nov 3, 2022 •

edited