Fix image loading for packaged apps

[jasonrclark]

SWT doesn't understand the embedded file paths that warbler creates
(uri:classloader), and so even when you give a properly calculated
relative path to an image in your JAR, it'd fail noisily.

To address this, we load the image from the file copy those files in
Ruby-land (which knows how to interpret those file paths) and always
just create images directly with the byte arrays. from those temp files.

What's remaining? TESTING! Did a bit but not satisfied yet, and don't
have the gumption to finish tonight so thought I'd get the PR rolling
anyway.

Fixes #676. Potentially also does the same for #1422 as long as images
were the only concern there.

• From shoes executable
  • Downloading file by URL
  • Image file from disk
• From packaged jar/app (per OS)
  • Windows
    • Downloading file by URL
    • Image file from disk
  • Linux
    • Downloading file by URL
    • Image file from disk
  • Mac
    • Downloading file by URL
    • Image file from disk
• Samples (at least that have image calls in them)!

[PragTob]

Cool stuff! I'm only a bit worried that it might end up causing us some troubles somewhere else but I don't hope so! Cool stuff, thank you @jasonrclark ! 🎉

[jasonrclark]

Yeah, as I've pondered it further I wonder how this will fare with memory and performance on large images. I'm planning to do more testing on this than usual...

Part of what I like about this approach is that it is always executed, which gets us tons more natural testing than something that only happens in packaged apps.

That said, if we hit problems with it at all, we could do like the downloading does, and simply copy any image referenced from uri:classloader (i.e. inside the package) out to a temporary location and then let SWT read from that file instead.

Anyway, will see what the testing looks like and report back!

[jasonrclark]

So, initial testing results (couldn't help myself 😀). Running with normal sized JPG images, I don't necessarily notice a huge difference, even when loading quite a few of them.

But to stress test things, I grabbed a 60MB JPG of the Hubble Deep Field.

# packaged-image-loading
real	0m8.217s
user	0m12.597s
sys	0m0.687s

# master
real	0m6.173s
user	0m8.490s
sys	0m0.923s

So that looks like a 50% slowdown on image loading... feels like that's significant warrant trying out the temp file approach instead. What do you think @PragTob.

[PragTob]

Hm. I'd like to see that benchmark to gauge it :D I.e. what are you measuring, the whole script? Just the image creation? :)

If possible I'd like to have a benchmark-ips of the creation of the image... but that might be hard-ish to get :|

Even then, 60MB is huge. If it really just occurs then it's fine imo as it's enough of an edge case for us to ignore atm (imo).

We could also roll with this and create a separate ticket to investigate performance, we should probably also add a sample to make sure that it keeps working even when we improve performance :)

[jasonrclark]

Yeah, this was just a quick benchmark by doing time around the app startup and shutting down as soon as it was visible. Will see if I can get something more repeatable whipped up, especially so we can see if the impact is the same on smaller images.

[PragTob]

Hm the problem might be graver then, if you really used time script.rb that'd include all of jruby/shoes startup which is already a couple of seconds and common to both. That'd mean that the image creation itself is much slower :(

[jasonrclark]

EDIT: Revised results posted below after realizing my flagging was running the wrong thing 😨

[jasonrclark]

As expected it was a bit of effort, but worth it, to get benchmarking closer around the image creation. This creates Shoes::Swt::Image objects, which runs the image load during initialization. Minor shuffling in feature code to flag the three approaches--straight filename (which doesn't work with relative names in packages), copying to tmp, and reading bytes directly through Ruby.

Rubocop is unhappy, but that's great since it will keep things red until we pick which one and resolve it on the branch.

Full results are over in this gist but I'll pick out the salient details:

# 1.3K file
small.png by raw bytes:  5726.7 i/s
small.png by filename:   2390.3 i/s - 2.40x  slower
small.png by tmp:        925.9 i/s - 6.19x  slower

# 194K
smallish.png by filename:  94.9 i/s
smallish.png by tmp:       86.6 i/s - 1.10x  slower
smallish.png by raw bytes: 75.6 i/s - 1.26x  slower

# 3.4M
medium.jpg by filename:  16.3 i/s
medium.jpg by tmp:       14.5 i/s - same-ish: difference falls within error
medium.jpg by raw bytes: 4.9 i/s - 3.32x  slower

# 61M
large.jpg by filename:   0.7 i/s
large.jpg by tmp:        0.6 i/s - 1.16x  slower
large.jpg by raw bytes:  0.3 i/s - 2.52x  slower

Interesting result here, the smallest file size was consistently faster with the byte reading approach this PR started with! But that clearly doesn't hold very long, as even up to about 200K for the file size the raw bytes is already much slower.

While the scaling isn't totally obvious, for anything of even moderate size, copying to a tmp location holds much closer to just letting SWT work off the filename (which we can't do within packages).

Upshot: I think we should take the tmpfile copying approach, but only do it when we detect that we're within a package and it's necessary. This keeps maximal performance with the normal execution, keeps as close as we can within packages, and the code is pretty straight-forward so I'm less worried about exercising it less often. That said, I need to extend the range of packaging samples that I use for releases this go around, since the existing single-file-no-images test I've been using isn't adequate anymore.

What say you @PragTob?

[PragTob]

I'd be inclined to use tmp files all the way. Performance hit for very small files is sort of big but we could still do 900 of them in a second which is good enough. Afterwards the performance is almost on par and I think I'd rather make that tradeoff than implementing 2 code paths, I'd argue it's more pragmatic/easier to maintain. Should it turn out to be a big issue we could still optimize performance later on.

Thanks for benchmarking ❤️

[jasonrclark]

I buy it... pushed up a largely untested (but Rubocop clear!) version of copying to tmp location along with cleanup. Done for the night with almost no testing, so that'll be what I pick up next. Ready to run the samples + packaging gauntlet!

[PragTob]

Some test doesn't wanna play:

  1) Shoes::Image basic should update image path

     Failure/Error: subject.path = updated_filename

     

     Errno::ENOENT:

       No such file or directory - /tmp/__shoes4_1494306729.684769_shoes-icon.png

     # ./shoes-swt/lib/shoes/swt/image.rb:143:in `block in cleanup_temporary_files'

     # ./shoes-swt/lib/shoes/swt/image.rb:142:in `cleanup_temporary_files'

     # ./shoes-swt/lib/shoes/swt/image.rb:63:in `create_image'

     # ./vendor/bundle/jruby/2.3.0/gems/after_do-0.4.0/lib/after_do.rb:99:in `block in create_image'

     # ./shoes-swt/lib/shoes/swt/image.rb:103:in `display_image'

     # ./shoes-swt/lib/shoes/swt/image.rb:38:in `load_image'

     # ./shoes-swt/lib/shoes/swt/image.rb:27:in `update_image'

     # ./shoes-core/lib/shoes/image.rb:23:in `path='

     # ./shoes-core/spec/shoes/image_spec.rb:21:in `block in (root)'

[jasonrclark]

Had wondered if I should wrap up that temp clean up in some rescues... looks like that's a "yes!"

[jasonrclark]

Updated to fix the tests which look like they'll be happy now. Also extended to spec out the behavior was seeing on Travis.

Looking to get to the testing this weekend and will remove the WIP when that gets done.

[PragTob]

some good updates!

[PragTob]

:)

[jasonrclark]

Found two issues in testing. First, we weren't making a temp copy of the static 'downloading' image that we show for URLs, so that borked when packaged.

Second, cleanup happens after the SWT image is made. But for a downloading image, we actually end up making two separate images--the downloading one and the other. Since we didn't clear the list after the downloading temp was deleted, we get a false warning on the second. Easy enough to address.

Bit more testing and then I think I'll get this merged tonight. I've noticed some other things around images + packaging that we can improve, but they're grounds for another PR.