5
\$\begingroup\$

I'm making a function that adds a new object to an array and removes the oldest if it has reached a specific length. Click the button in the example below. All objects are drawn until its array reaches a length of 5 when the oldest is removed.

Is there a better, more processor and memory efficient way to do this instead of needlessly throwing the oldest entry at the garbage collector?

let c = document.getElementById("context");
let ctx = c.getContext("2d");
let particles = [];

function addParticle() {
  particles.push({
    x: Math.random() * 800,
    y: Math.random() * 500,
    width: Math.random() * 100 + 50,
    height: Math.random() * 100 + 50,
    color: '#' + Math.floor(Math.random() * 16777215).toString(16)
  });

  if (particles.length > 5) particles.shift();

  draw();
}

function draw() {
  ctx.clearRect(0, 0, 800, 500);

  for (let i = 0; i < particles.length; i++) {
    ctx.fillStyle = particles[i].color;
    ctx.fillRect(particles[i].x, particles[i].y, particles[i].width, particles[i].height);
  }
}
<button onclick="addParticle();">Add</button>
<canvas id="context" width="800" height="500"></canvas>

\$\endgroup\$
  • 1
    \$\begingroup\$ Your title should explain what your code does, not the changes you want to be made to it, or what performance "features" it has. \$\endgroup\$ – FreezePhoenix Sep 25 '20 at 12:46
  • \$\begingroup\$ @FreezePhoenix thank you for pointing that out. I've changed the title :) \$\endgroup\$ – Will Pierce Sep 25 '20 at 12:54
  • 1
    \$\begingroup\$ What does "needlessly throwing the oldest entry at the garbage collector" mean? In what way is it "needless"? \$\endgroup\$ – RoToRa Sep 25 '20 at 12:56
  • \$\begingroup\$ @RoToRa it's needless because the array's length is capped to a specific number. Re-using objects, if possible, will incur less GC. The effects of garbage memory is noticeable when this function is run frequently. \$\endgroup\$ – Will Pierce Sep 25 '20 at 13:05
  • 3
    \$\begingroup\$ I really doubt that any effects of garbage collection are even measurable in this example. Generating the random numbers and drawing the rectangles probably take up more time than the memory handling of those few objects. Have you run some benchmarks? \$\endgroup\$ – RoToRa Sep 25 '20 at 13:25
4
\$\begingroup\$

I think the current code is just fine from an efficiency standpoint. The overhead of a single small object is essentially nothing. Even if 1000 such objects were created and then got GC'd every time the button was clicked, on any remotely modern device, the performance impact would almost certainly be imperceptible.

If you really wanted to be more efficient, it might be very slightly more efficient to use a rotating modulo index that gets used to assign the new object instead of re-shuffling the array indicies with unshift, but with only 5 elements in the array, it's not something worth worrying about. Better to worry about performance tweaks only if

  • The improvement is easy to implement and doesn't make the code harder to read, or
  • You've tested the script on a low-end device and know that something isn't running as fast as it should - in which case you should run a check to see what section(s) of the code are causing the bottleneck(s), and then work on improving only those bottlenecks.

IMO, anything else is premature optimization - your efforts are better spent elsewhere, such as making the code clean and readable, which usually matters far more than performance.

And there are a number of improvements you could make on the code quality front:

Use const by default - only use let when you must reassign the variable name. Using const makes the code more readable when one can identify at a glance that something isn't going to be reassigned.

Avoid inline handlers - they have a whole bunch of problems, including a demented scope chain which virtually requires global pollution, mixing page content with presentation, and ugly escaping issues. When you have an event listener you want to attach to an element, better to use addEventListener.

Abstract iteration When iterating, unless you actually need to work with the index, it's more abstract and more readable to work with the element being iterated over directly instead. For example, rather than particles[i].width, it'd be nicer to use particle.width, or even just width extracted from the particle. You can do this by invoking Array#forEach or the array's iterator.

ID You have <canvas id="context" width="800" height="500"></canvas>. But the canvas is not the 2d context - giving the element an ID of context is somewhat misleading. Also, IDs create global variables by default, unfortunately, which can sometimes lead to hard-to-understand bugs. You can select the canvas with querySelector('canvas') instead. (If you had a larger page and collisions were possible, you could give the element unique classes instead; classes do not create global identifiers.)

const button = document.querySelector('button');
button.addEventListener('click', addParticle);

const ctx = document.querySelector('canvas').getContext("2d");
const particles = [];

function addParticle() {
  particles.push({
    x: Math.random() * 800,
    y: Math.random() * 500,
    width: Math.random() * 100 + 50,
    height: Math.random() * 100 + 50,
    color: '#' + Math.floor(Math.random() * 16777215).toString(16)
  });

  if (particles.length > 5) particles.shift();

  draw();
}

function draw() {
  ctx.clearRect(0, 0, 800, 500);
  particles.forEach(({ color, x, y, width, height }) => {
    ctx.fillStyle = color;
    ctx.fillRect(x, y, width, height);
  });
}
<button>Add</button>
<canvas width="800" height="500"></canvas>

\$\endgroup\$
  • \$\begingroup\$ After testing, I found that the native shift was better. However, the forEach slowed down performance - even without using destructuring assignment. \$\endgroup\$ – Will Pierce Sep 27 '20 at 2:45
3
\$\begingroup\$

Edit 2
Specific to your code/side note of main question:
You can create multiple canvas layers.
See "Use multiple layered canvases for complex scenes" in Optimizing canvas - Web APIs | MDN
End Edit 2

Edit 1
Alternatively:

particles.length==5?[, ...particles]=[...particles, particle]:particles.push(particle);

var particles=[];

for(var i=0; i<10; i++) {
  var particle=Math.random().toFixed(5);
  particles.length==5?[, ...particles]=[...particles, particle]:particles.push(particle);
  console.log("["+particles.join(", ")+"]");
};
.as-console-wrapper { max-height: 100% !important; top: 0; }

End Edit 1

As per my comment:

  if(particles.length<5) {
    particles.push(particle);
  } else {
    for(var i=1; i<5; i++) {
      particles[i-1]=particles[i];
    }
    particles[4]=particle;
  }

let c = document.getElementById("context");
let ctx = c.getContext("2d");
let particles = [];

function addParticle() {
  var particle={
    x: Math.random() * 800,
    y: Math.random() * 500,
    width: Math.random() * 100 + 50,
    height: Math.random() * 100 + 50,
    color: '#' + Math.floor(Math.random() * 16777215).toString(16)
  };
  if(particles.length<5) {
    particles.push(particle);
  } else {
    for(var i=1; i<5; i++) {
      particles[i-1]=particles[i];
    }
    particles[4]=particle;
  }

  draw();
}

function draw() {
  ctx.clearRect(0, 0, 800, 500);

  for (let i = 0; i < particles.length; i++) {
    ctx.fillStyle = particles[i].color;
    ctx.fillRect(particles[i].x, particles[i].y, particles[i].width, particles[i].height);
  }
}
<button onclick="addParticle();">Add</button>
<canvas id="context" width="800" height="500"></canvas>

\$\endgroup\$
3
\$\begingroup\$

Main question

Is there a better, more processor and memory efficient way to do this instead of needlessly throwing the oldest entry at the garbage collector?

It seems that calling shift() is the fastest way to remove the first element1

While it is likely not going to make any noticeable difference when iterating over an array with five elements, the order in which particles are drawn doesn’t matter so one small improvement would be to loop backwards - i.e. instead of:

   for (let i = 0; i < particles.length; i++) {

Start i at the end of the array and decrement it until it reaches zero:

for (let i = particles.length; i--; /* intentional no-op */) {

i will first be assigned particles.length and then be decremented by the condition expression before the loop statements are executed.

With this approach the length of the array is only referenced once, and the post-execution operation is eliminated because the decrease is the counter happens in the pre-test condition.

Review Points

Good things

  • function names are appropriate and concise
  • lines are terminated properly
  • white space is used appropriately

Suggestions

Variable declarations

As suggested in the answer by CertainPerformance any variable that does not need to be re-assigned can be declared with const instead of let to avoid accidental re-assignment. This works for the array particles since it is never re-assigned.

Variable names

Names like c are vague. A more appropriate name would be canvas.

Hard-coded numbers

Is 16777215 formulated somehow? Perhaps it should be declared as a constant.

Instead of hard-coded values for the height and width used in addParticle()

x: Math.random() * 800,
y: Math.random() * 500,

values of the canvas could be used:

x: Math.random() * canvas.width,
y: Math.random() * canvas.height,
\$\endgroup\$
  • 1
    \$\begingroup\$ Hello. This is not inline with the op's intentions. "Removing unneeded objects from an array before rendering" and "All objects are drawn until its array reaches a length of 5 when the oldest is removed" implied: from the array, not the canvas. Also, a bug(?): when a drawn object is "removed" from canvas, it 'takes away' portions of other drawn objects if they were unfortunate enough to be drawn 'underneath' it. Out of curtesy, I did not down-vote your answer. \$\endgroup\$ – iAmOren Oct 1 '20 at 9:43
  • 1
    \$\begingroup\$ @iAmOren thank you for pointing that out in a courteous manner. I have removed that alternate approach. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 1 '20 at 12:32
  • 1
    \$\begingroup\$ Of course! I treat others the way I'd like to be treated. I get so many down-vote without reason (and if I express what I really think about this and them, I'll be suspended again...). \$\endgroup\$ – iAmOren Oct 1 '20 at 13:34
  • \$\begingroup\$ for (let i = particles.length; i--; /* intentional no-op */) { - confuses me: Won't it point to "after" the array (as it's zero-based-indexed(?))? (The "intentional no-op" part is clear: the test condition will fail when i is zero) \$\endgroup\$ – iAmOren Oct 1 '20 at 13:36
  • \$\begingroup\$ the condition expression is evaluated before each loop iteration so on the first iteration i will be assigned particles.length and then decremented in the condition expression \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 1 '20 at 14:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.