COMP: as per memononen's suggestions, tweak findTexture #452

Chaircrusher · 2018-01-22T21:01:19Z

This is based closely on the suggestion you gave to me, but tweaked to work a little better.

The salt is shifted once to the high word. Right-shifting & left-shifting was messing up on ints.
I make the salt by caling rand; it appears that you were taking it from uninitialized memory, which isn't a reliable way to get unique salt for each texture.
I made sure the salt was nonzero, because if the salt was zero and the ID was zero, it makes findTexture look like it failed.

This solves the 'stale texture ID' problem:

If it's a valid ID, the salt matches.
If it's a deleted ID, the salt in the array is zero, and the salt in the ID doesn't match.
If it's a texture that's been deleted and re-used, the salt won't match.

This version has slightly more overhead than the original, (mostly on allocTexture when it matters least), but has better overall performance due to skipping a sequential search for textures by ID.

cforfang · 2018-01-23T16:30:15Z

src/nanovg_gl.h


 	for (i = 0; i < gl->ntextures; i++) {
 		if (gl->textures[i].id == 0) {
-			tex = &gl->textures[i];
+			idx = i;
 			break;
 		}
 	}


tex == NULL below this line should probably be idx == -1 now?

EDIT:
Couldn't find a way to comment on the exact line, but it's the if below the first loop.

That's correct, below should be:

if (idx == -1) {

cforfang · 2018-01-23T16:47:41Z

src/nanovg_gl.h

@@ -363,38 +364,54 @@ static GLNVGtexture* glnvg__allocTexture(GLNVGcontext* gl)
 			gl->textures = textures;
 			gl->ctextures = ctextures;
 		}
-		tex = &gl->textures[gl->ntextures++];
+		idx = gl->ntextures++;


Instead of using rand() for the salt below, if you're here you know you're initializing a new entry for which you can set salt = 0.

cforfang · 2018-01-23T16:48:37Z

src/nanovg_gl.h

@@ -147,6 +147,7 @@ struct GLNVGtexture {
 	int width, height;
 	int type;
 	int flags;
+	unsigned int salt;


Isn't salt already stored as part of the id?

Well... thinking about it. The goal is to make the ID == the index in the array, so storing the ID in the Texture structure is no longer necessary.

The salt is what need to be stored. The ID returned to the application is the ID + the salt.

cforfang · 2018-01-23T16:58:52Z

Right-shifting & left-shifting was messing up on ints.

I didn't think this was a problem, but then I also don't use bitfields much :) What problems did you encounter?

it appears that you were taking it from uninitialized memory, which isn't a reliable way to get unique salt for each texture

This seems like a bug in @memononen comment. I think you only need to identify when a new entry is allocated and set the salt to 0 then. If you add that I think the original code (which increments the salt on delete) should work.

Also, from skimming the code it's not clear to me why a separate salt field is needed. Is it not sufficent to just store it as part of id?

Chaircrusher · 2018-01-23T17:22:39Z

I didn't think this was a problem, but then I also don't use bitfields much :) What problems did you
encounter?

The problems was using ints. When you right-shift a signed integer, it propogates the sign bits left. This means that for some numbers X << 16 != (X << 16) >> 16.

I solved the problem by just making the salt a random integer left-shifted into the high two bytes.

This seems like a bug in @memononen comment. I think you only need to identify when a new entry is allocated and set the salt to 0 then. If you add that I think the original code (which increments the salt on delete) should work.

Two things: 1) the existing code thinks an ID of zero means allocTexture failed, which means ID can never be zero. 2) The salt could be an incrementing counter, but it should not start at zero, because the index can be zero, so 0 + 0 violates assumption #1

Also, from skimming the code it's not clear to me why a separate salt field is needed. Is it not sufficent to just store it as part of id?

As I said above, the ID is no longer needed as part of the Texture struct.

I'll submit a new version of this branch that A) takes out the id from the Texture struct and B) uses monotonically increasing salt values.

memononen

Bitshifts should work just fine, I think casting to unsigned int is a good idea here. Maybe int does some sign expansion. Let's keep the API with int, though.

There was an oversight in my code, the salt should be initialized to 1. That way valid IDs are always > 0, and 0 can be invalid id. I'm pretty sure that is not used in the code base, but it is part of the common salted index pattern.

memononen · 2018-01-23T17:17:34Z

src/nanovg_gl.h


 	for (i = 0; i < gl->ntextures; i++) {
 		if (gl->textures[i].id == 0) {
-			tex = &gl->textures[i];
+			idx = i;
 			break;
 		}
 	}


That's correct, below should be:

if (idx == -1) {

memononen · 2018-01-23T17:18:22Z

src/nanovg_gl.h

-	GLNVGtexture* tex = NULL;
-	int i;
+ 	GLNVGtexture* tex = NULL;
+	int i, idx = -1, salt;


int i, idx = -1, salt = 1;

The salt as a local variable in allocTexture means that it isn't preserved across calls. I made it a member of GLNVGcontext as textureSalt.

memononen · 2018-01-23T17:19:17Z

src/nanovg_gl.h


 	for (i = 0; i < gl->ntextures; i++) {
 		if (gl->textures[i].id == 0) {
-			tex = &gl->textures[i];
+			idx = i;


idx = i; salt = gl->textures[i].salt; // Carry salt from previous item

memononen · 2018-01-23T17:19:49Z

src/nanovg_gl.h

 	}

+	tex = &gl->textures[idx];
+	while(!(salt = (unsigned int)rand()))


Remove rand() stuff.

memononen · 2018-01-23T17:20:16Z

src/nanovg_gl.h

-	tex->id = ++gl->textureId;
+	tex->salt = salt;
+	// salt value is in high 2 bytes, ID in low two bytes
+	tex->salt <<= 16;


Remove
tex->salt <<= 16;

memononen · 2018-01-23T17:21:08Z

src/nanovg_gl.h

+	tex->salt = salt;
+	// salt value is in high 2 bytes, ID in low two bytes
+	tex->salt <<= 16;
+	tex->id = (idx & 0xffff) | tex->salt;


Maybe the bitshift problem was operator precedence, try:

tex->id = (idx & 0xffff) | (tex->salt << 16);

memononen · 2018-01-23T17:22:10Z

src/nanovg_gl.h

+		glDeleteTextures(1, &tex->tex);
+	memset(tex, 0, sizeof(*tex));
+	// salt always > 0, so deleted texture will have illegal salt
+	tex->salt = 0; 


Salt must be increment here, not set to zero.

memononen · 2018-01-23T17:22:39Z

src/nanovg_gl.h

-	}
-	return 0;
-}
+	int idx = id & 0xffff, salt = ((unsigned int)id) & 0xffff0000;


The bitshift should work here too no problem.

Chaircrusher · 2018-01-23T17:29:44Z

working on a new version. will update within the hour.

cforfang · 2018-01-23T17:37:09Z

The problems was using ints. When you right-shift a signed integer, it propogates the sign bits left. This means that for some numbers X << 16 != (X << 16) >> 16.

But is that a problem in this case? I know left shifts of signed integers is undefined, but I wasn't sure if right-shifts for the purpose of extracting two numbers like here is OK.

Chaircrusher · 2018-01-23T17:49:47Z

I checked in a new version. I think that it is simple and correct. I've verified that it works properly in the application I was testing in (vcvRack). If you want to do it differently be my guest.

memononen · 2018-01-24T06:31:17Z

src/nanovg_gl.h

-	tex->id = ++gl->textureId;
+	// pre-increment so salt begins at one. When GLNVGcontext is
+	// allocated initially it is set to 0 via memset.
+	tex->salt = ++gl->textureSalt;


The reason for using a salt per texture entry is that they are less likely to wrap around and keep unique. I'd prefer to use that version.

COMP: as per memononen's suggestions, tweak findTexture

1a47ddf

cforfang reviewed Jan 23, 2018

View reviewed changes

memononen requested changes Jan 23, 2018

View reviewed changes

COMP: incorporated suggestions from nanovg developers

7dbc018

memononen reviewed Jan 24, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COMP: as per memononen's suggestions, tweak findTexture #452

COMP: as per memononen's suggestions, tweak findTexture #452

Chaircrusher commented Jan 22, 2018

cforfang Jan 23, 2018 •

edited

Loading

memononen Jan 23, 2018

cforfang Jan 23, 2018

cforfang Jan 23, 2018

Chaircrusher Jan 23, 2018

cforfang commented Jan 23, 2018

Chaircrusher commented Jan 23, 2018

memononen left a comment

memononen Jan 23, 2018

memononen Jan 23, 2018

Chaircrusher Jan 23, 2018

memononen Jan 23, 2018

Chaircrusher Jan 24, 2018

memononen Jan 23, 2018

memononen Jan 23, 2018

memononen Jan 23, 2018

memononen Jan 23, 2018

memononen Jan 23, 2018

Chaircrusher commented Jan 23, 2018

cforfang commented Jan 23, 2018

Chaircrusher commented Jan 23, 2018

memononen Jan 24, 2018

COMP: as per memononen's suggestions, tweak findTexture #452

Are you sure you want to change the base?

COMP: as per memononen's suggestions, tweak findTexture #452

Conversation

Chaircrusher commented Jan 22, 2018

cforfang Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cforfang commented Jan 23, 2018

Chaircrusher commented Jan 23, 2018

memononen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chaircrusher commented Jan 23, 2018

cforfang commented Jan 23, 2018

Chaircrusher commented Jan 23, 2018

Choose a reason for hiding this comment

cforfang Jan 23, 2018 •

edited

Loading