Skip to content

ObjectPool: Create Pool Size of 100 instead of throwing on 0#163

Merged
derrickstolee merged 2 commits intomicrosoft:masterfrom
derrickstolee:bug162
Aug 14, 2018
Merged

ObjectPool: Create Pool Size of 100 instead of throwing on 0#163
derrickstolee merged 2 commits intomicrosoft:masterfrom
derrickstolee:bug162

Conversation

@derrickstolee
Copy link
Contributor

A user reported an issue when cloning a repo with no files (other than .gitignore and .gitattributes). This is recorded by #162.

The issue is that the ObjectPool throws an exception when given a pool size of 0, and we multiply the number of index entries by a double less than 1.0 and cast down to an int. This is easily fixed by adding a minimum pool size. This is verified with a simple unit test.

@kewillford
Copy link
Member

Does the LazyUTF8Pool or the BytePool need similar minimums? There multipliers are greater than 1.0 so they wouldn't cause an error but might be good just to use a minimum on them as well? Also thinking about this more and the expand might never increase if the pool is too small.

@derrickstolee
Copy link
Contributor Author

@kewillford What do you think about changing my approach here and replacing the exception in ObjectPool with a minimum? There's nothing special about the minimum of 100 and the duplicated logic in 6 places makes me concerned. The tests can stay.

@kewillford
Copy link
Member

@derrickstolee Yeah that is much simpler. Looks good.

@derrickstolee derrickstolee changed the title SortedFolderEntries: Never initialize below 100 ObjectPool: Create Pool Size of 100 instead of throwing on 0 Aug 14, 2018
{
LazyUTF8String.ResetPool(new MockTracer(), 0);
LazyUTF8String.FreePool();
LazyUTF8String.InitializePools(new MockTracer(), 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also validate that LazyUTF8String.Length is 100 after ResetPool and InitializePools?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about validating, but the number 100 is not special (and in the new implementation is hidden).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could validate that it is greater than 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think we can add a verification that the value is strictly larger than 0. Which is the important thing, and gives a clear indicator of "what are we testing here?" (instead of just "don't throw").

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm not used to GitHub not updating automatically while the browser is open.)

{
SortedFolderEntries.FreePool();
SortedFolderEntries.InitializePools(new MockTracer(), indexEntryCount: 0);
SortedFolderEntries.ResetPool(new MockTracer(), indexEntryCount: 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here, validate that the size is 100?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants