Add 2-component vector constructor #1569
Conversation
| if (FFlag::LuauVector2Constructor && !vectorSrc.empty()) | ||
| { | ||
| std::string what = "create: @checked (x: number, y: number, z: number) -> vector"; | ||
| std::string replacement = "create: @checked (x: number, y: number, z: number?) -> vector"; | ||
| std::string::size_type pos = vectorSrc.find(what); | ||
| LUAU_ASSERT(pos != std::string::npos); | ||
| vectorSrc.replace(pos, what.size(), replacement); | ||
| } |
There was a problem hiding this comment.
This implies that after unflagging, the code will forever do a string substitution of an incorrect definition.
Instead, the code should switch the whole definition string so that after flag is removed we only have one simple definition with no runtime fixups.
| @@ -1055,25 +1057,25 @@ static int luauF_tunpack(lua_State* L, StkId res, TValue* arg0, int nresults, St | |||
|
|
|||
| static int luauF_vector(lua_State* L, StkId res, TValue* arg0, int nresults, StkId args, int nparams) | |||
There was a problem hiding this comment.
This implementation looks worrying, I would like to see benchmarks on the most common 3 component case it was focused before in LUA_VECTOR_SIZE 3. 3/4 for LUA_VECTOR_SIZE 4 as a bonus.
It also includes many unflagged changes.
As a suggestion for flagging this, consider having if (flag){ full new implementation } else { full old implementation }
tests/Compiler.test.cpp
Outdated
| LOADK R0 K0 [1, 2, 3] | ||
| RETURN R0 1 | ||
| )"); | ||
|
|
||
| CHECK_EQ("\n" + compileFunction("print(Vector3.new(1, 2, 3))", 0, 2, 0, /*enableVectors*/ true), R"( | ||
| CHECK_EQ("\n" + compileFunction("print(vector.create(1, 2, 3))", 0, 2, 0, /*enableVectors*/ true), R"( |
There was a problem hiding this comment.
Having 'enableVectors' set doesn't actually make sense for a vector.create test.
I would leave at least one test for legacy constructors, but we did want to update some tests to vector.create without enableVectors now that library is available.
|
Thanks for the comments. Pushed a revision that addresses all the points mentioned. Also added a new micro benchmark for vector library. Test results: LuauVector2Constructor false:
LuauVector2Constructor true:
Command line used: Tested on: Windows 10, MSVC 2022, Intel i5-3570K The difference is a bit less than 1% in favor of the old version. I don't think this will make a big difference in practice though. I also ran the test with only two arguments (the new LuauVector2Constructor path):
Creating vectors from two components is now much faster as expected (previously ~218ms on average with a user defined 2D constructor without the fastcall). An idea for future work: we could consider adding a new built-in for the 2D case (luauF_vector2). This would get rid of the extra branch in luauF_vector and get back that 1% and make luauF_vector2 even faster. This would require detecting the number of arguments in the compiler and choosing the correct builtin. Not sure if it makes sense to add complexity for such a small gain though. |
Implement RFC: 2-component vector constructor. This includes 2-component overload for
vector.createand associated fastcall function, and its type definition. These features are controlled by a new feature flagLuauVector2Constructor. Additionally constant folding now supports two components whenLuauVector2Constantsfeature flag is set.Note: this work does not include changes to CodeGen. Thus calls to
vector.createwith only two arguments are not natively compiled currently. This is left for future work.