stewartlord / identicon.js

GitHub-style identicons as PNGs or SVGs in JS
BSD 2-Clause "Simplified" License
1.31k stars 131 forks source link

Synchronous image generation and performance #30

Closed mrkvon closed 2 years ago

mrkvon commented 7 years ago

The Identicon.toString() method is synchronous. Generating an image in node may be an expensive action. This may lead to blocking code.

Is there any performance issue when generating images of size i.e. 512*512px? Does any benchmark exist?

I wonder what would be the performance impact if the image was generated on a server dynamically on every request.

I hope the question is clear and good enough for GitHub issues. Otherwise please kindly ask or redirect me to a proper place.

buildbreakdo commented 7 years ago

@mrkvon Pass a true boolean into .toString(true) which returns an SVG. You can losslessly scale SVG up or down to any size as it is a vector graphic. So you can make a 40x40 and then set the size to 512x512 and it will look exactly the same.

Do not have an answer for your other questions, although I would avoid pre-optimizing. Optimize when you bump into a performance issue, otherwise optimize for readability/maintainability of code. Cannot really optimize anything without profiling given you are trying to predict how Node is going to optimize your code and that is a moving target.

buildbreakdo commented 7 years ago

Hah.. bumped into this problem too. Looks like because absolute values are used in svg > g > rect scaling with CSS does not work (least not at first glance). Solution (aside from fixing the lib, which it should be fixed!) is to use

svg g {
  transform: scale(3);
}

Would have the same affect.

mrkvon commented 7 years ago

@buildbreakdo Thanks a lot for clarification.

I would prefer to serve png. (To keep design simpler. The identicons would be served as default and user-provided-images would be png or jpeg.).

I see svg has a great browser support, too. I'll either go with that, or cache on-signup-generated icons on disc.

Currently, generating png dynamically seems to slow down my dev server significantly. This is just an impression, though. I'm too bfu to provide any hard statistical performance data now.

Thanks for clarifying and sorry for replying so late.

stewartlord commented 7 years ago

Thanks @buildbreakdo, I will look at making the values scalable

stewartlord commented 7 years ago

@mrkvon I would suggest generating the images client-side. That will eliminate the network transfer and the server-side processing effort.

stewartlord commented 7 years ago

Hi @buildbreakdo, setting image width/height via CSS is scaling the image for me. Do you have repro steps?

buildbreakdo commented 7 years ago

@stewartlord Statement was poorly worded, clearly can scale with CSS, end of that post even provided example of how to do it. If memory serves, believe what was being said was that a relative measure is not being used which automatically scales with font-size.

Common practice to use font-size to drive other sizes of things (as it is inherited down the DOM tree). Em measures key off of the last declared font-size. For example, if the last declared is font-size:10px then width and height are set set to 50em the width and height would be 50px. Font-size declarations set the base number of what 1 em unit equals. All browsers default to 16px on HTML elements, most sites reset with a 10px or 62.5% [62.5% of default 16px is 10px] declaration on the element because base having 1em equal to 1px is much easier to talk about with devs.

What using EM units to drive sizing does is give one place to change one declaration (font-size) that then cascades to elements below. Basically drive sizing of things (including identicons) a single font-size change. Makes downsizing or upsizing for different device viewports centralized and declarative, more maintainable (less JavaScript). Chris Coyier has a good article on the topic: https://css-tricks.com/why-ems/

Ask was to have identicons be em based as well. Problem with this is you don't know what base is being keyed off of, do they have 10px base font or 16px? So makes little sense. Gut says there is a way, not sure if further investigation is really justifiable.

buildbreakdo commented 7 years ago

@stewartlord Okay, think I got it.

Looking at this:

var options = {
      foreground: [0, 0, 0, 255],               // rgba black
      background: [255, 255, 255, 255],         // rgba white
      margin: 0.2,                              // 20% margin
      size: 420                                 // 420px square
      format: 'svg'                             // use SVG instead of PNG
    };

Right now, the unit of measure is a bit magical (so are /rgba?/ values), pass in a number and sometimes outputs a number in units of pixels, sometimes a number in units of %. This is not explicit, if it was made explicit, as in people could pass in 1em or 20px, then supporting use of em values would make sense, if the user is using em values then we can fairly assume they know the visual size that will be output. Imagine this becomes an issue with PNG generation though where a specific px size is being generated? So this would be SVG only (again not sure this is worth the cycles, but is possible)?

To keep things backward compatible, if no size unit is passed then the lib would perform the current default behaviour.

stewartlord commented 7 years ago

Ooh! I like it :+1: I'll take a crack at it unless you are already working on it?

buildbreakdo commented 7 years ago

@stewartlord Go for it, swamped right now, need another pair of eyes at any point happy to help.

stewartlord commented 2 years ago

Closing due to inactivity