Duke4.net Forums: Submitting patches for code? - Duke4.net Forums

Jump to content

Hide message Show message
Welcome to the Duke4.net Forums!

Register an account now to get access to all board features. After you've registered and logged in, you'll be able to create topics, post replies, send and receive private messages, disable the viewing of ads and more!

Page 1 of 1
  • You cannot start a new topic
  • You cannot reply to this topic

Submitting patches for code?

User is offline   Jaap 

  • 50

#1

 
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

This post has been edited by Jaap: 13 November 2016 - 03:14 AM

3

User is offline   Hendricks266 

  • EDuke32 Senior Developer
  • 4,937

#2

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:

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.
0

User is offline   Jaap 

  • 50

#3

Hi Hendricks266,

Long time no see.

View PostHendricks266, 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.


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.

View PostHendricks266, 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:


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.
0

User is offline   Hendricks266 

  • EDuke32 Senior Developer
  • 4,937

#4

View PostJaap, 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.

View PostJaap, 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.
0

Share this topic:


Page 1 of 1
  • You cannot start a new topic
  • You cannot reply to this topic


All copyrights and trademarks are property of their respective owners and all comments are owned by their posters. Yes, our forum uses cookies. © 2004-2017 Duke4.net and Voidpoint, LLC

Enter your sign in name and password


Sign in options