Hi guys,
Is there an official way to submit patches for the code? I made a small improvement to the logging of "setarray" so you can see if you are trying to set set an array out of bounds you can see which one it is. I would also like a function to remove an entire range from an array at once. I have already coded one but I would like it to become part of Eduke so anyone can use it and people wont be stuck to my versions of eduke in order to play my mods.
Patch for the better out of bounds message
http://www.jaapvande...xec.c.patch.zip
Patch for the con function that removes a range from an array:
http://www.jaapvande...Range.patch.zip
- Jaap
Page 1 of 1
Submitting patches for code?
#1 Posted 12 November 2016 - 07:20 AM
This post has been edited by Jaap: 13 November 2016 - 03:14 AM
#2 Posted 01 March 2017 - 01:23 AM
Hi Jaap,
I finally had a chance to glance down my to-do list, and I was able to look at your patches.
The error message patch is a good feature improvement, but there is something else that we must consider. The code that executes the CON files is what is known as a "hot path"--it is very important to the overall performance of EDuke32 that this code executes as fast as possible. One of the biggest threats to performance on modern processors is conditional branching--if statements. The information printed should let you be able to tell which of the two error conditions is the problem, so adding this extra check would slow down the setarray command everywhere, even if they are correct. However, if I understand correctly, the "invalid array ID" error can only come from an internal bug in the CON compiler, so that message should be removable as long as we can guarantee correctness elsewhere.
The patch that adds a new CON command to remove ranges from arrays is an interesting idea. I like that it has the potential to perform an operation on the C side faster than the corresponding CON code. My concern with adding it is exactly how special-case an operation it is. For example, the setarray command performs this step:
1. set array[index] equal to value
resizearray performs these steps:
1. allocate a new array of size newSize
2. copy min(oldSize, newSize) elements from the old array to the new array
3. fill the rest with zeroes if the new size is larger than the smaller
4. modify the gamearray data structure to point to the new array instead of the old one
5. free the old array
An ideal removearrayrange would do this:
1. allocate a new array of size oldSize - (rangeEnd - rangeStart)
2. copy [0, rangeStart) from the old array to [0, rangeStart) of the new array
3. copy [rangeEnd, oldSize) to [rangeStart, newSize)
4. modify the gamearray data structure to point to the new array instead of the old one
5. free the old array
But what if you wanted to remove multiple ranges from an array? What if you wanted to halve the size, by only keeping every other element? Steps 2 and 3 could become many more steps.
The best scripting languages will provide you with small and modular building blocks to be able to perform any operation you can dream of. A command like removearrayrange is too big of a building block. The only major operation that CON cannot do is swap the pointers of arrays, so that you only need to copy all the data once instead of twice (which is an improvement to your patch you may want to research how to do for your own learning). If we had this, the implementation would look something like:
This will still be slower than the C code, but only because of the overhead of an interpreted language, which is generally acceptable.
I finally had a chance to glance down my to-do list, and I was able to look at your patches.
The error message patch is a good feature improvement, but there is something else that we must consider. The code that executes the CON files is what is known as a "hot path"--it is very important to the overall performance of EDuke32 that this code executes as fast as possible. One of the biggest threats to performance on modern processors is conditional branching--if statements. The information printed should let you be able to tell which of the two error conditions is the problem, so adding this extra check would slow down the setarray command everywhere, even if they are correct. However, if I understand correctly, the "invalid array ID" error can only come from an internal bug in the CON compiler, so that message should be removable as long as we can guarantee correctness elsewhere.
The patch that adds a new CON command to remove ranges from arrays is an interesting idea. I like that it has the potential to perform an operation on the C side faster than the corresponding CON code. My concern with adding it is exactly how special-case an operation it is. For example, the setarray command performs this step:
1. set array[index] equal to value
resizearray performs these steps:
1. allocate a new array of size newSize
2. copy min(oldSize, newSize) elements from the old array to the new array
3. fill the rest with zeroes if the new size is larger than the smaller
4. modify the gamearray data structure to point to the new array instead of the old one
5. free the old array
An ideal removearrayrange would do this:
1. allocate a new array of size oldSize - (rangeEnd - rangeStart)
2. copy [0, rangeStart) from the old array to [0, rangeStart) of the new array
3. copy [rangeEnd, oldSize) to [rangeStart, newSize)
4. modify the gamearray data structure to point to the new array instead of the old one
5. free the old array
But what if you wanted to remove multiple ranges from an array? What if you wanted to halve the size, by only keeping every other element? Steps 2 and 3 could become many more steps.
The best scripting languages will provide you with small and modular building blocks to be able to perform any operation you can dream of. A command like removearrayrange is too big of a building block. The only major operation that CON cannot do is swap the pointers of arrays, so that you only need to copy all the data once instead of twice (which is an improvement to your patch you may want to research how to do for your own learning). If we had this, the implementation would look something like:
gamearray tempbuf 0 gamearray myArray 512 gamevar temp 0 0 defstate remove_top_half getarraysize myArray temp divvar temp 2 resizearray tempbuf temp copy myArray[temp] tempbuf[0] temp // command does not currently exist swaparrays tempbuf myArray resizearray tempbuf 0 ends
This will still be slower than the C code, but only because of the overhead of an interpreted language, which is generally acceptable.
#3 Posted 01 March 2017 - 02:22 AM
Hi Hendricks266,
Long time no see.
Actually, if you look at how the if statements are structured they are not on the happy flow of the "hot path". Only if things go wrong one extra if statement is hit. The main thing my change is that it adds the name of the array was being used. Useful information while debugging your code. I have had many instances where the line number was not much use because multiple files had an setarray on that line number.
Good point but this does not work for algorithms that require you to remove a range that lies in the range of an array without having to do multiple copy actions. While adding the swap command would enable others to make their own implementation of removerange it will make removing a range from an array quite more complex than (I feel) it would have to be and would still have the overhead of doing two copies.
Long time no see.
Hendricks266, on 01 March 2017 - 01:23 AM, said:
Hi Jaap,
The error message patch is a good feature improvement, but there is something else that we must consider. The code that executes the CON files is what is known as a "hot path"--it is very important to the overall performance of EDuke32 that this code executes as fast as possible. One of the biggest threats to performance on modern processors is conditional branching--if statements. The information printed should let you be able to tell which of the two error conditions is the problem, so adding this extra check would slow down the setarray command everywhere, even if they are correct. However, if I understand correctly, the "invalid array ID" error can only come from an internal bug in the CON compiler, so that message should be removable as long as we can guarantee correctness elsewhere.
The error message patch is a good feature improvement, but there is something else that we must consider. The code that executes the CON files is what is known as a "hot path"--it is very important to the overall performance of EDuke32 that this code executes as fast as possible. One of the biggest threats to performance on modern processors is conditional branching--if statements. The information printed should let you be able to tell which of the two error conditions is the problem, so adding this extra check would slow down the setarray command everywhere, even if they are correct. However, if I understand correctly, the "invalid array ID" error can only come from an internal bug in the CON compiler, so that message should be removable as long as we can guarantee correctness elsewhere.
Actually, if you look at how the if statements are structured they are not on the happy flow of the "hot path". Only if things go wrong one extra if statement is hit. The main thing my change is that it adds the name of the array was being used. Useful information while debugging your code. I have had many instances where the line number was not much use because multiple files had an setarray on that line number.
Hendricks266, on 01 March 2017 - 01:23 AM, said:
The patch that adds a new CON command to remove ranges from arrays is an interesting idea. I like that it has the potential to perform an operation on the C side faster than the corresponding CON code. My concern with adding it is exactly how special-case an operation it is. For example, the setarray command performs this step:
1. set array[index] equal to value
resizearray performs these steps:
1. allocate a new array of size newSize
2. copy min(oldSize, newSize) elements from the old array to the new array
3. fill the rest with zeroes if the new size is larger than the smaller
4. modify the gamearray data structure to point to the new array instead of the old one
5. free the old array
An ideal removearrayrange would do this:
1. allocate a new array of size oldSize - (rangeEnd - rangeStart)
2. copy [0, rangeStart) from the old array to [0, rangeStart) of the new array
3. copy [rangeEnd, oldSize) to [rangeStart, newSize)
4. modify the gamearray data structure to point to the new array instead of the old one
5. free the old array
But what if you wanted to remove multiple ranges from an array? What if you wanted to halve the size, by only keeping every other element? Steps 2 and 3 could become many more steps.
The best scripting languages will provide you with small and modular building blocks to be able to perform any operation you can dream of. A command like removearrayrange is too big of a building block. The only major operation that CON cannot do is swap the pointers of arrays, so that you only need to copy all the data once instead of twice (which is an improvement to your patch you may want to research how to do for your own learning). If we had this, the implementation would look something like:
1. set array[index] equal to value
resizearray performs these steps:
1. allocate a new array of size newSize
2. copy min(oldSize, newSize) elements from the old array to the new array
3. fill the rest with zeroes if the new size is larger than the smaller
4. modify the gamearray data structure to point to the new array instead of the old one
5. free the old array
An ideal removearrayrange would do this:
1. allocate a new array of size oldSize - (rangeEnd - rangeStart)
2. copy [0, rangeStart) from the old array to [0, rangeStart) of the new array
3. copy [rangeEnd, oldSize) to [rangeStart, newSize)
4. modify the gamearray data structure to point to the new array instead of the old one
5. free the old array
But what if you wanted to remove multiple ranges from an array? What if you wanted to halve the size, by only keeping every other element? Steps 2 and 3 could become many more steps.
The best scripting languages will provide you with small and modular building blocks to be able to perform any operation you can dream of. A command like removearrayrange is too big of a building block. The only major operation that CON cannot do is swap the pointers of arrays, so that you only need to copy all the data once instead of twice (which is an improvement to your patch you may want to research how to do for your own learning). If we had this, the implementation would look something like:
Good point but this does not work for algorithms that require you to remove a range that lies in the range of an array without having to do multiple copy actions. While adding the swap command would enable others to make their own implementation of removerange it will make removing a range from an array quite more complex than (I feel) it would have to be and would still have the overhead of doing two copies.
#4 Posted 01 March 2017 - 11:55 AM
Jaap, on 01 March 2017 - 02:22 AM, said:
Actually, if you look at how the if statements are structured they are not on the happy flow of the "hot path".
Branches harm performance simply by existing.
Jaap, on 01 March 2017 - 02:22 AM, said:
While adding the swap command would enable others to make their own implementation of removerange it will make removing a range from an array quite more complex than (I feel) it would have to be and would still have the overhead of doing two copies.
The complexity of this operation belongs in the CONs. It does not have any more copy operations than the equivalent C code would.
Share this topic:
Page 1 of 1