Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track GPU memory usage #984

Merged
merged 10 commits into from Mar 19, 2019

Conversation

Projects
None yet
3 participants
@tsherif
Copy link
Member

commented Mar 14, 2019

  • Create a global statsManager singleton that allows independent objects to add their contributions to given stats.
  • Track memory usage of buffers, textures and renderbuffers
  • Add a memory stats widget to the 3D texture example

Key design question I wouldn't mind getting some feedback on is the StatsManager. Couldn't think of another way to keep memory usage stats in one place. Let me know if you guys can think of alternatives.

tsherif added some commits Mar 14, 2019

@tsherif tsherif requested review from ibgreen and Pessimistress Mar 14, 2019

@ibgreen
Copy link
Contributor

left a comment

Very nice, looking forward to being able to see data usage in the stats widget!

What do you think about storing the stats object on the context instead of globally? We already have a little luma object on the context where we store other state so it would fit pretty nicely in there.

Might also be worth pointing out that all the WebGL classes inherit from Resource so you might be able to put some supporting method there to keep the code minimal.

class Resource {
  trackGPUMemoryUsage(byteLength, resourceCategory) {
    this.gpuMemoryStats = statsManager.get('Memory Usage').get('GPU Memory');
    this.bufferMemoryStats = statsManager.get('Memory Usage').get(resourceCategory);
 }
}
@@ -29,13 +30,22 @@ export default class Renderbuffer extends Resource {

constructor(gl, opts = {}) {
super(gl, opts);
this.gpuMemoryStats = statsManager.get('Memory Usage').get('GPU Memory');

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Contributor

Would it make sense to define constants for these strings and decouple them from the printable representation? Or would that create too much coupling in the code?

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 14, 2019

Contributor

Or this could be done in the base Resource class constructor. We may also add a method resource.setMemoryUsage or something.

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 14, 2019

Author Member

Yeah, it would make sense to move at least parts of this into Resource. With the byteLength naming convention, it could easily manage everything except the per-type stats.

@@ -90,4 +108,38 @@ export default class Renderbuffer extends Resource {
// this.gl.bindRenderbuffer(GL.RENDERBUFFER, null);
return value;
}

/* eslint-disable complexity */
_getFormatSize() {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Contributor

Idea: We already have a table for formats, maybe reuse that or add this field to that table?

See top of file:

import RENDERBUFFER_FORMATS from './renderbuffer-formats';

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 14, 2019

Author Member

Good call. And since the formats are the same between textures and renderbuffers, I could have one for both.

data,
parameters = {}
}) {
this.gpuMemoryStats.subtractCount(this.byteLength);

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Contributor

Maybe call super() and then add stats instead of moving here?

// TODO(Tarek): Cover all formats/types?
const numTexels = width * height * (depth || 1);
let channels;
switch (dataFormat) {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Contributor

Again worth looking if we can reuse existing tables?

@@ -3,6 +3,7 @@ import Resource from './resource';
import Accessor from './accessor';
import {assertWebGL2Context, getGLTypeFromTypedArray, getTypedArrayFromGLType} from '../utils';
import {log, assert, checkProps} from '../../utils';
import statsManager from '../../core/stats-manager';

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Contributor

Note: Buffers can be reallocated, also check setData/_setData to follow how they reinitialize buffers. Might want to keep things updated.

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 14, 2019

Author Member

So I believe this is covered by the fact it always subtracts the current byteLength from the counts before adding to the count after allocation. Is there a path that I'm missing?

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 14, 2019

Contributor

OK. Worth double checking Buffer._setByteLength I think it can change the size of the buffer.

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 15, 2019

Author Member

Ah good catch! Wasn't covering the reallocate method. Moving tracking into _setData and _setByteLength

@tsherif

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

@ibgreen can you take another look at this? Primary changes are:

  • Move total GPU memory tracking into Resource methods _trackDeallocatedMemory and _trackAllocatedMemory, subclasses override to track type-specific stats.
  • Added pixelSize, texelSize, channels entries to TEXTURE_FORMATS and RENDERBUFFER_FORMATS tables to be used for size calculations.
  • Filled out entries in the TEXTURE_FORMATS table to ensure coverage of most types we might use.
  • TYPE_SIZES table in texture-formats module, used for size calculations.
  • Fixed specs to handle new formats in the TEXTURE_FORMATS table.
  • Added memory stats to instancing example.

@tsherif tsherif requested a review from ibgreen Mar 15, 2019

@ibgreen
Copy link
Contributor

left a comment

Summary: Bundle size worries relating to table extensions, please check comments make measurements, and open issue tracking a fix before landing.

@@ -152,12 +152,27 @@ class AppAnimationLoop extends AnimationLoop {
}
});

return {cloud, mvpMat, viewMat, statsWidget};
const memWidget = new StatsWidget(statsManager.get('Memory Usage'), {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

Side note:

My proposal is that we cut this widget from the individual examples and add it to the website.

That way we get the benefits in all demos without cluttering up every app.

I made a big push on the website in the pending scenegraph split PR, should be able to go through how it would work once that lands.

@@ -154,13 +162,36 @@ class AppAnimationLoop extends AnimationLoop {
}
});

return {statsWidget};
const memWidget = new StatsWidget(statsManager.get('Memory Usage'), {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

Side note: My initial thinking is that we would have one widget that can show multiple screens and can be expanded/collapsed/circulated by the user, rather than add multiple widgets. Zero config in apps and all power in the users hands to slice and dice the data. Such sophistication would come later of course.

[GL.RG32I]: {gl2: true},
[GL.RGB8]: {gl2: true},
[GL.RGBA8]: {gl2: true},
[GL.R8]: {gl2: true, bytesPerPixel: 1},

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

Tables take a fair bit of space in the bundle, I have always had some angst about these. And I believe that key strings i.e. {bytesPerPixel: ...} are usually not minified like variable names (worth checking). I'm normally a big proponent of typing things out but in this case maybe something shorter (bpp?), or use arrays and destructure on use.

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 16, 2019

Author Member

I assumed the minification would work if it's not exported externally? Not something I know too well though, so I'll just change it to bpp

@@ -29,13 +30,20 @@ export default class Renderbuffer extends Resource {

constructor(gl, opts = {}) {
super(gl, opts);
this.renderbufferMemoryStats = statsManager.get('Memory Usage').get('Renderbuffer Memory');

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

Is this lookup an cheap operation? If so, maybe not save this member on the object?

@@ -284,4 +289,14 @@ export default class Resource {

stats.resourceMap[name].active--;
}

_trackAllocatedMemory(bytes) {

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

Nit: Probably a lot of extra work, but assume it could be valuable to see what type of resource is using the bytes? I.e. add one more param?

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 16, 2019

Author Member

That's what's happening in the overrides in the subclasses.

// R
// [GL.R8]: {dataFormat: GL.RED, types: [GL.UNSIGNED_BYTE], gl2: true},

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

So this was commented out for bundle size reasons. Would be good if you could run collect metrics before and after this change. And make a pass on making the table smaller. That could be a second PR, no need to hold this one, but let's not just add this without checking.

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 16, 2019

Author Member

Ok, sure. I'm thinking what I'll do is create another table DATA_FORMAT_CHANNELS, which will be quite small, and then I can do the calculations for most cases based on numChannels * typeSize.

Might not be an issue for our current use cases, but it seems odd that we're returning "not supported" for a lot of useful WebGL 2 formats

if (info.gl1 === undefined && info.gl2 === undefined) {
// No info - always supported
return true;
}
const value = isWebGL2(gl) ? info.gl2 || info.gl1 : info.gl1;
const value = isWebGL2(gl) ? info.gl2 : info.gl1;

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

I believe this was written this way to save space and not duplicate gl2 entries if gl1 was identical. Clearly there should have been comments indicating this.

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 16, 2019

Author Member

The issue I had was formats that exist in WebGL 1 but not 2, mainly the depth texture extension formats.

if (data && data.byteLength) {
this._trackAllocatedMemory(data.byteLength);
} else {
let bytesPerTexel = TEXTURE_FORMATS[this.format].bytesPerTexel;

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 16, 2019

Contributor

Maybe helper methods to calculate this from the table, both here and in other file?

@@ -0,0 +1,17 @@
import {Stats} from 'probe.gl';

class StatsManager {

This comment has been minimized.

Copy link
@Pessimistress

Pessimistress Mar 16, 2019

Contributor

There is a global object luma.stats that tracks the count of resource objects by type. Worth consolidating?

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 16, 2019

Author Member

Definitely! Hadn't notice that.

@tsherif

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

@ibgreen @Pessimistress Take 3!

  • Consolidate stat tracking in global luma.stats, which is now a "stats manager"-like object.
  • Revert TEXTURE_FORMAT table updates. Create a DATA_FORMAT_CHANNELS table that can be used with TYPE_SIZES to calculate byte-per-pixel for textures.
  • Remove subclass _trackAllocatedMemory and _trackDeallocatedMemory methods using a pattern similar to what happens in Resource._addStats.

@tsherif tsherif requested review from Pessimistress and ibgreen Mar 18, 2019

@ibgreen
Copy link
Contributor

left a comment

Minor nit: Maybe import the global when used instead of adding it to eslintrc. I feel that putting it in the linter rules kind of hides what is going on when looking at the files for someone who is not familiar with the code base.

@@ -10,7 +10,7 @@ import {getPageLoadPromise} from '../webgl/context';
import {isWebGL, requestAnimationFrame, cancelAnimationFrame} from '../webgl/utils';
import {log} from '../utils';
import assert from '../utils/assert';
import {Stats} from 'probe.gl';
import luma from '../init';

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 18, 2019

Contributor

If we import it like this, whenever we use it, we don't need to add it to .eslintrc?

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 18, 2019

Author Member

The issue I had was in the examples, since the luma object isn't exported from Luma.gl. Should we export it?

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 18, 2019

Contributor

Should we export it?

In that case we should probably export the stats object, not the luma wrapper object.

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 19, 2019

Author Member

Sounds good.

const data = withData ? TEXTURE_DATA[type] || DEFAULT_TEXTURE_DATA : null;
const options = Object.assign({}, formatInfo, {
data,
format,
type,
mipmaps: format !== GL.RGB32F, // TODO: for some reason mipmap generation failing for RGB32F format
mipmaps: formatInfo.dataFormat === GL.RGBA, // Avoid mipmapping compressed formats

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 18, 2019

Contributor

Is GL.RGBA true for GL.R etc? Maybe a bit more comment here?

This comment has been minimized.

Copy link
@tsherif

tsherif Mar 18, 2019

Author Member

Ah, this is left over from when the TEXTURE_FORMATS table was expanded. I'll revert.

tsherif added some commits Mar 19, 2019

@tsherif tsherif merged commit 8cecc03 into master Mar 19, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.