wurstscript
@unchecked annotation or other solutions to checked dispatch bloat
What the title says.
Right now, checked dispatch is inserted at every class method call, heavily bloating up the code, and dragging down the excellent inlining system that has been put in place.
We already have @inline, but unfortunately, for certain situations the bloat from checked dispatch is so big, that even this annotation does not force a function to be always inlined. It is especially frustrating when I am relying on the inliner to elide unnecessary conditional branches, when the compiler is perfectly capable of doing so.
This is especially bad for private functions in classes, because the only place where private functions can get called from are other public methods, typically after a check has already passed.
The only case where it can mis-fire is if you destroy the class instance inside one method, and then try to invoke said private function, or the class is accidentally destroyed by other code.
I do not want to disable checked dispatch for my entire project, however, as I think it is an immensely useful utility even in release mode, where I want most performance. But, still, the bloat from it is sometimes so obscene that it is quite worrying indeed. I can have a function that is 95% dispatch checks and only 5% actual logic.
Thus, I ask for an @unchecked annotation for class methods that allows disabling checked dispatch for certain methods. That way, I, as a programmer, can have fine-grained control over when dispatch checking is disabled, ensuring that the methods that I want inlined, are getting inlined, and get rid of the quite obscene overhead that is incurred by these checks. Obviously, it can be a dangerous feature and easily abused, but I think it is better than disabling dispatch checks in the entire project at once.
Alternatively, it may be used as a conditional directive that is only invoked when it is most useful - e.g. when inlining is enabled, because it is something that benefits the most from it.
I can imagine there are more elegant ways of solving this, but my biggest problem is that it can negatively interfere with the inliner, forcing it to give up on inlining a function in checked mode, even when there are great benefits from doing so (removing dead branches, merging code, etc.)
If required, I can provide a code sample where it is very relevant.
Maybe we can disable the checks for all calls on "this", since (as you said) it's very unlikely that a class destroys itself before the end of a method.
The optimal solution would of course be a better optimizer that removes all unnecessary checks.
Maybe we can disable the checks for all calls on "this", since (as you said) it's very unlikely that a class destroys itself before the end of a method.
I don't think so. it's actually increasingly common, the more you work with inheritance and bigger systems. A function that might be overridden somewhere by some super/subclass you don't even know might call a destroy. (happens also with other functions/callback not happening due to super. side effects.)
I often also do it by accident, simply calling destroy before calling another method.
My main issues are:
- The check is only done for functions. you can still access variables without an error, this is inconsistent and confusing af
- There is no proper way provided by the language to check if an instance is "dead". The typeId == 0 or so works (and is used by wurst itself for the check) but iirc u said it's unsafe and might be changed.
- The error breaks the entire thread. Imo it should either be configurable via a runarg, or just be changed, to simply ignore calls on the destroyed instance or just the specific function and show an error, instead of breaking the thread. Breaking the thread has broken my maps several times ingame, because instructions after the error were not executed anymore.
- This doesn't throw an error at compiletime:
let a = new A()
destroy a
a.doSomething()
The check is only done for functions. you can still access variables without an error, this is inconsistent and confusing af
Agree, but the overhead seemed too much for variables. If we have better optimization, we can add that check.
There is no proper way provided by the language to check if an instance is "dead". The typeId == 0 or so works (and is used by wurst itself for the check) but iirc u said it's unsafe and might be changed.
This is because we reuse ids -- after an object is destroyed, it might be reused and you cannot distinguish the case where 42 points to the new object or the old destroyed object. The proper way to program with destroy is to clear all references to an object before destroying it. If you cannot do this, just add a flag isDestroyed to your class and use this instead of destroying the object.
The error breaks the entire thread. Imo it should either be configurable via a runarg, or just be changed, to simply ignore calls on the destroyed instance or just the specific function and show an error, instead of breaking the thread. Breaking the thread has broken my maps several times ingame, because instructions after the error were not executed anymore.
I think it can be changed in the standard library by adapting ErrorHandling.error which is marked as configurable. However, I don't think this is a good idea, since you then are in an inconsistent state and all the followup errors might obscure the actual bug. If we really want to be more robust against errors, we should add something like try-catch-finally to the language... (but maybe we should have this discussion on a different thread)
This doesn't throw an error at compiletime:
It does now :)
Agree, but the overhead seemed too much for variables. If we have better optimization, we can add that check.
Yea seems reasonable
I think it can be changed in the standard library by adapting ErrorHandling.error which is marked as configurable. However, I don't think this is a good idea, since you then are in an inconsistent state and all the followup errors might obscure the actual bug. If we really want to be more robust against errors, we should add something like try-catch-finally to the language... (but maybe we should have this discussion on a different thread)
I think most of these errors pop up in places where you wouldn't use try/catch. And yes, it might be an inconsistent state, but I would argue that first, this is the case in vjass and in wurst for variables(rn) and second, in most cases, it's a simple thing, like missing a != null check before a destroy, that breaks the entire thread. Perhaps certain low severity action like double frees (or just, destroying null) should not cause a thread crash.
Other thread for discussing the error-handling thing: https://github.com/wurstscript/WurstScript/issues/732
For the current issue, I suggest that we wait for examples and then see if we want to invest in better optimization for the example or have the suggested annotation as a workaround.
Bear with me, but this is the example I have. Consider this code:
package Hello
import Table
enum HashType
INTEGER
REAL
STRING
BOOLEAN
function HashType.toString() returns string
string result
switch this
case INTEGER
result = "integer"
case REAL
result = "real"
case STRING
result = "string"
default
result = "boolean"
return result
public class HashReader
private Table data = new Table
private var integerReadIndex = 0
private var realReadIndex = 0
private var stringReadIndex = 0
private var booleanReadIndex = 0
private var integerCount = 0
private var realCount = 0
private var stringCount = 0
private var booleanCount = 0
construct()
@inline
function hasRemainingData(HashType t) returns boolean
boolean result
switch t
case INTEGER
result = integerCount > integerReadIndex
case REAL
result = realCount > realReadIndex
case STRING
result = stringCount > stringReadIndex
default
result = booleanCount > booleanReadIndex
return result
@inline
function getReadIndex(HashType t) returns int
int result
switch t
case INTEGER
result = integerReadIndex
case REAL
result = realReadIndex
case STRING
result = stringReadIndex
default
result = booleanReadIndex
return result
@inline
function getCount(HashType t) returns int
int result
switch t
case INTEGER
result = integerCount
case REAL
result = realCount
case STRING
result = stringCount
default
result = booleanCount
return result
// when checked dispatch is disabled and all opts are on, this will compile down to very simple checks
@inline
function validateType(HashType t)
if not hasRemainingData(t)
print("trying to read " + t.toString() + " at position " + getReadIndex(t).toString() + " out of " + getCount(t).toString())
@inline
function readInt() returns int
validateType(HashType.INTEGER)
return readIntUnchecked()
@inline
function readIntUnchecked() returns int
let temp = data.loadInt(integerReadIndex)
integerReadIndex++
return temp
@noinline
function inline_remainingData(HashReader test)
print("remaining data (int): " + test.hasRemainingData(HashType.INTEGER).toString())
@noinline
function inline_readIndex(HashReader test)
print("readIndex (int): " + test.getReadIndex(HashType.INTEGER).toString())
@noinline
function inline_getCount(HashReader test)
print("getCount(int): " + test.getCount(HashType.INTEGER).toString())
@noinline
function inline_validateType(HashReader test)
print("validateType(int)")
test.validateType(HashType.INTEGER)
@noinline
function inline_readInt(HashReader test)
print("readInt: " + test.readInt().toString())
@noinline
function inlineTests()
let test = new HashReader()
inline_remainingData(test)
inline_readIndex(test)
inline_getCount(test)
inline_validateType(test)
inline_readInt(test)
init
inlineTests()
This is a lot of code, but it is precisely in these kind of scenarios where this issue manifests itself.
The implementation of validateType(HashType) (and other similar functions) relies on the assumption that on high optimization levels, it will be inlined and dead branches eliminated. And indeed, this is exactly what happens if we turn on -localOptimizations -inline -nodebug -uncheckedDispatch. Take a look:
function inline_remainingData takes integer test returns nothing
local string cond_result
if HashReader_integerCount[test] > HashReader_integerReadIndex[test] then
set cond_result = "true"
else
set cond_result = "false"
endif
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "remaining data (int): " + cond_result)
endfunction
function inline_readIndex takes integer test returns nothing
set test = HashReader_integerReadIndex[test]
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "readIndex (int): " + I2S(test))
endfunction
function inline_getCount takes integer test returns nothing
set test = HashReader_integerCount[test]
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "getCount(int): " + I2S(test))
endfunction
function inline_validateType takes integer test returns nothing
local string temp
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "validateType(int)")
if HashReader_integerCount[test] <= HashReader_integerReadIndex[test] then
set temp = "trying to read integer at position " + I2S(HashReader_integerReadIndex[test]) + " out of "
set test = HashReader_integerCount[test]
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., temp + I2S(test))
endif
endfunction
function inline_readInt takes integer test returns nothing
local integer this
local string temp
if HashReader_integerCount[test] <= HashReader_integerReadIndex[test] then
set this = HashReader_integerReadIndex[test]
set temp = "trying to read integer at position " + I2S(this) + " out of "
set this = HashReader_integerCount[test]
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., temp + I2S(this))
endif
set this = HashReader_data[test]
set this = LoadInteger(Table_ht, this, HashReader_integerReadIndex[test])
set HashReader_integerReadIndex[test] = HashReader_integerReadIndex[test] + 1
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "readInt: " + I2S(this))
endfunction
It does exactly what it needs to, eliminating dead branches and making the code only perform the checks that it knows will be performed anyway.
The reason why I'm using this technique, is because the only other way is to make 4 hasRemainingData() functions and 4 validateType() functions. That is, unfortunately, a lot of boilerplate, and I'm trying to use the optimizations to my advantage here.
Now, take a look without -uncheckedDispatch:
function inline_getCount takes integer test returns nothing
if HashReader_typeId[test] == 0 then
if test == 0 then
call error("Nullpointer exception when calling HashReader.getCount")
else
call error("Called HashReader.getCount on invalid object.")
endif
endif
set test = HashReader_integerCount[test]
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "getCount(int): " + I2S(test))
endfunction
function inline_readIndex takes integer test returns nothing
if HashReader_typeId[test] == 0 then
if test == 0 then
call error("Nullpointer exception when calling HashReader.getReadIndex")
else
call error("Called HashReader.getReadIndex on invalid object.")
endif
endif
set test = HashReader_integerReadIndex[test]
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "readIndex (int): " + I2S(test))
endfunction
function inline_readInt takes integer test returns nothing
if HashReader_typeId[test] == 0 then
if test == 0 then
call error("Nullpointer exception when calling HashReader.readInt")
else
call error("Called HashReader.readInt on invalid object.")
endif
endif
call dispatch_HashReader_Hello_HashReader_validateType(test, 0)
set test = dispatch_HashReader_Hello_HashReader_readIntUnchecked(test)
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "readInt: " + I2S(test))
endfunction
function inline_remainingData takes integer test returns nothing
local string cond_result
if dispatch_HashReader_Hello_HashReader_hasRemainingData(test, 0) then
set cond_result = "true"
else
set cond_result = "false"
endif
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "remaining data (int): " + cond_result)
endfunction
function inline_validateType takes integer test returns nothing
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "validateType(int)")
call dispatch_HashReader_Hello_HashReader_validateType(test, 0)
endfunction
After a certain level of nesting, functions stopped getting inlined. But, it gets even worse if you look inside e.g. dispatch_HashReader_Hello_HashReader_validateType
function dispatch_HashReader_Hello_HashReader_validateType takes integer this, integer t returns nothing
local string result
if HashReader_typeId[this] == 0 then
if this == 0 then
call error("Nullpointer exception when calling HashReader.validateType")
else
call error("Called HashReader.validateType on invalid object.")
endif
endif
if not dispatch_HashReader_Hello_HashReader_hasRemainingData(this, t) then
if t == 0 then
set result = "integer"
elseif t == 1 then
set result = "real"
elseif t == 2 then
set result = "string"
else
set result = "boolean"
endif
call DisplayTimedTextToPlayer(Player_localPlayer, 0., 0., 45., "trying to read " + result + " at position " + I2S(dispatch_HashReader_Hello_HashReader_getReadIndex(this, t)) + " out of " + I2S(dispatch_HashReader_Hello_HashReader_getCount(this, t)))
endif
endfunction
Yikes. Even with the @inline annotations, they still do not get inlined. The only difference between these two examples is that one was compiled with uncheckedDispatch, and the other without.
Sure, I could work around this by making 4 functions instead of 1, etc. etc., but I've been trying to take advantage of the optimizer to avoid boilerplate, and this is just one (comparatively small) example of how checked dispatch drags down the capabilities of the inliner and the optimizer. I don't think it's hard to imagine that there are many more examples even in Stdlib where stuff doesn't get inlined (and misses out on various other optimizations as a result) because of checked dispatch bloat.
I understand that @unchecked would be but a stop-gap measure to solve this, and I agree, but it is better than not using checked dispatch at all. I always check the emitted JASS code for my libraries, and try to tweak them if I see anything amiss. Many modern languages provide 'unsafe' facilities one way or another, and yes, they are mis-used, but they are also a powerful tool, that when used by a responsible developer can lead to much greater benefit than not.
Alternatively, I guess this also illustrates what a good use-case for some kind of macro system would be.
Imho it just shows missed optimization potential resp. dispatch checks messing with inlining for some reason and that those should be improved. Adding a workaround seems somewhat counterproductive. I already see all funcs in your code having 2 annotations ;)