patternjavascriptMinor
Finding Memoryleaks In Data Structures
Viewed 0 times
datastructuresmemoryleaksfinding
Problem
Looking for constructive criticism on my code below for detecting a memory leak in a data structure.
MemoryLeak = {
uniq_id: (new Date()).getTime(),
checked: 1,
is_seen: [],
checkLeaks: function(obj) {
var self = MemoryLeak
if(!obj || (typeof obj == 'function') || self.checked > 20000)
return;
if ((self._isArray(obj) || self._isObject(obj))) {
if (self._isArray(obj)) {
self._logTooBig(obj, obj.length)
for (var i = 0; i 200) {
console.log('Object too big, memory leak? [size: ' + limit + ']')
console.log(obj)
console.log('-------')
}
};
_keys: function(obj) {
var rval = [], prop
for (prop in obj)
rval.push(prop)
return rval;
};
_isArray: function(obj) {
try {
return obj instanceof Array
}
catch(e) {
return false;
}
};
_isObject: function(obj) {
return (typeof obj == 'object')
};
_partial: function(fn) {
var args = Array.prototype.slice.call(arguments)
args.shift()
return function() {
var new_args = Array.prototype.slice.call(arguments)
args = args.concat(new_args)
return fn.apply(window, args)
}
}
}Solution
_logTooBig: function(obj, limit) {
if (limit > 200) {
console.log('Object too big, memory leak? [size: ' + limit + ']')
console.log(obj)Naming: Surely the parameter you've called
limit ought to be named size! The word "limit" implies an upper bound, as in "speed limit". In this case, the "limit" on object size is 200. The parameter you've currently called limit is the thing you're supposed to be comparing to the size limit. What it actually represents is just a size.Separately, are you at all worried that repeatedly
console.logging (only) huge objects will be a bad debugging experience? Or do good browsers already handle huge objects just fine?var self = MemoryLeak;
self.checked++Syntax nit: missing semicolon after
++. This is legal of course, but I think many linters will flag it as an inconsistency.Why do you do this thing with
self = MemoryLeak instead of just saying this.checked++? I might be exposing my own lack of knowledge here, but I thought this was supposed to do the right thing in "member" functions called with the MemoryLeak.foo() syntax. Is your use of self actually working around a problem with this, or is it just an idiosyncrasy that could be cleaned up?try {
return obj instanceof Array
}
catch(e) {
return false;
}Again I may be exposing my own lack of knowledge, but I'm about 75% sure that
instanceof cannot possibly throw an exception. What case are you worried about here?_keys: function(obj) {
var rval = [], prop
for (prop in obj)
rval.push(prop)
return rval;
};This should at least be rewritten
_keys: function(obj) {
var rval = [];
for (var prop in obj)
rval.push(prop);
return rval;
};and (unless I'm missing something) it should simply be
_keys: function(obj) { return Object.keys(obj); }if(!obj || (typeof obj == 'function') || self.checked > 20000)
return;I see where you initialize
checked = 1 and where you checked++, but I don't see anywhere that checked's value decreases or is reset. So do you only ever check 20000 entities and then stop checking forever? That doesn't seem like super useful behavior, since memory leaks typically manifest only after a fairly long time. What's the point of the checked counter? (If you keep it, then it would be a good idea to add a //code comment explaining why it's desirable. Code comments are great for explaining the "why" of code.)Speaking of "explain the why": it would have been nice to see an example of how you expect the user to use this leak checker. I gather that he'd just insert a call to
MemoryLeak.checkLeaks(mySuspiciousObject) after initializing the object, and that would trigger the checking of mySuspiciousObject every 5 seconds for 20000 iterations?Certainly exposing my ignorance: I wasn't aware that all your
};s were legal. I thought you'd have to make them },.Code Snippets
_logTooBig: function(obj, limit) {
if (limit > 200) {
console.log('Object too big, memory leak? [size: ' + limit + ']')
console.log(obj)var self = MemoryLeak;
self.checked++try {
return obj instanceof Array
}
catch(e) {
return false;
}_keys: function(obj) {
var rval = [], prop
for (prop in obj)
rval.push(prop)
return rval;
};_keys: function(obj) {
var rval = [];
for (var prop in obj)
rval.push(prop);
return rval;
};Context
StackExchange Code Review Q#155618, answer score: 2
Revisions (0)
No revisions yet.