This repository has been archived by the owner. It is now read-only.

Fix serialization format for mtime on files #428

Merged
merged 1 commit into from Apr 9, 2013

Conversation

Projects
None yet
3 participants
@SamSaffron
Copy link
Contributor

SamSaffron commented Apr 8, 2013

I noticed that file digests were being calculated like madness EVERY request in dev mode.

BHSTWZGCEAExhAM

This all boils down to mtimes being serialized with a serializer that was chucking out precision.

This fix is backwards compatible, though to get proper performance users are encouraged to nuke the /tmp/cache folder.

Results for Discourse in Dev.

Before:

before

After:

after

That is a 110ms improvement. There is another 20ms or so lurking in porting the .stat calls to pure mtime, cause you don't really need full stats.


Ruby Time ... the more you know:

[1] pry(main)> t=Time.now;t==Time.at(t.to_i)
=> false

[2] pry(main)> t=Time.now;t==Time.parse(t.iso8601)
=> false

[3] pry(main)> t=Time.now;t==Time.at(t.to_r)
=> true
@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 8, 2013

fixing the failures hold tight

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 8, 2013

I amended this to the much simpler to_i coercion, plenty of other spots treat time with 1 second fidelity, I think it is fine to keep it consistent

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Apr 8, 2013

@SamSaffron what do you think about changing the cache serialization format too? There was a possible issue regarding its performance in #390. Maybe we could just use an int there instead.

@ghost ghost assigned josh Apr 8, 2013

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 8, 2013

Yeah I agree, will revise, that parse is pointless, Time.at is more efficient though the hike patch will do a rescue to make sure it continues to work on old /tmp folders

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Apr 8, 2013

When we bump Sprockets::VERSION, that will invalidate old /tmp caches. So we're free to change the format without any backwards compat.

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 8, 2013

Ok, done @josh , now for my hike patch, so I can fix the mtime stuff here ... the stat is too expensive

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 9, 2013

Ok, another major win

image

Was noticing @trail.stat was really slow, at first I though mtime would be faster, but looking at the flamegraph it was not mtime vs stat that was the problem, it was the huge amount of initialization going on. If we dig through hike, non of it is needed:

    #
    def index
      Index.new(root, paths, extensions, aliases)
    end


    # `Trail#stat` is equivalent to `File#stat`. It is not
    # recommend to use this method for general purposes. It exists for
    # parity with `Index#stat`.
    def stat(*args)
      index.stat(*args)
    end

#-- Index

  # directly, create a `Trail` and call `Trail#index`.
    def initialize(root, paths, extensions, aliases)
      @root = root

      # Freeze is used here so an error is throw if a mutator method
      # is called on the array. Mutating `@paths`, `@extensions`, or
      # `@aliases` would have unpredictable results.
      @paths      = paths.dup.freeze
      @extensions = extensions.dup.freeze
      @aliases    = aliases.inject({}) { |h, (k, a)|
                      h[k] = a.dup.freeze; h
                   }.freeze
      @pathnames  = paths.map { |path| Pathname.new(path) }

      @mtimes   = {}
      @stats    = {}
      @entries  = {}
      @patterns = {}
    end

 # A cached version of `File.stat`. Returns nil if the file does
    # not exist.
    def stat(path)
      key = path.to_s
      if @stats.key?(key)
        @stats[key]
      else
        begin
          @stats[key] = File.stat(path)
        rescue Errno::ENOENT
          @stats[key] = nil
        end
      end
    end

So much uneeded work just to get a File.stat, root is ignored anyway, cache is bypassed. Its just pointless work.


After the change:

image

# note, @trail.stat is sub-optimal
# it create a new Index in hike, which initiliazed a bunch of
# unused stuff, the stat cool in Index disregards root anyway
File.stat(path)

This comment has been minimized.

@josh

josh Apr 9, 2013

Contributor

Anyway we can speed Trail#stat up? Its nice thats its a cached call.

This comment has been minimized.

@SamSaffron

SamSaffron Apr 9, 2013

Contributor

Thing is its not, unless it keeps the Index class it generates around, so
"fixing" this will change the current behavior people expect from the call

On Tue, Apr 9, 2013 at 11:57 AM, Joshua Peek notifications@github.comwrote:

In lib/sprockets/base.rb:

@@ -228,7 +228,10 @@ def entries(pathname)
#
# Subclasses may cache this method.
def stat(path)

  •  @trail.stat(path)
    
  •  # note, @trail.stat is sub-optimal
    
  •  # it create a new Index in hike, which initiliazed a bunch of
    
  •  # unused stuff, the stat cool in Index disregards root anyway
    
  •  File.stat(path)
    

Anyway we can speed Trail#stat up? Its nice thats its a cached call.


Reply to this email directly or view it on GitHubhttps://github.com//pull/428/files#r3706535
.

This comment has been minimized.

@josh

josh Apr 9, 2013

Contributor

I think this helps the development uncached code path, but penalizes production cached calls.

This comment has been minimized.

@SamSaffron

SamSaffron Apr 9, 2013

Contributor

Thing is the call graph is the following:

@trail.stat(path) => stat => Index.new => stat

Every trail.stat call is uncached. We could cache that Index.new ... but it would change the way the API is designed.

This comment has been minimized.

@josh

josh Apr 9, 2013

Contributor

Sprockets::Index#stat could still invoke @trail.stat, but Sprockets::Environment could invoke the uncached File.stat.

This comment has been minimized.

@josh

josh Apr 9, 2013

Contributor

Now I'm thinking it might be better to patch Hike.

This comment has been minimized.

@SamSaffron

SamSaffron Apr 9, 2013

Contributor

yeah ... maybe Trail#stat should simply shadow and File#stat call in hike,
then none of this judo is needed and its functionally equivalent anyway.

Naming wise its pretty confusing that @trail is holding Trail sometimes and
Index in other times ..

On Tue, Apr 9, 2013 at 12:32 PM, Joshua Peek notifications@github.comwrote:

In lib/sprockets/base.rb:

@@ -228,7 +228,10 @@ def entries(pathname)
#
# Subclasses may cache this method.
def stat(path)

  •  @trail.stat(path)
    
  •  # note, @trail.stat is sub-optimal
    
  •  # it create a new Index in hike, which initiliazed a bunch of
    
  •  # unused stuff, the stat cool in Index disregards root anyway
    
  •  File.stat(path)
    

Now I'm thinking it might be better to patch Hike.


Reply to this email directly or view it on GitHubhttps://github.com//pull/428/files#r3706811
.

This comment has been minimized.

@josh

josh Apr 9, 2013

Contributor

Check it - sstephenson/hike#23

This comment has been minimized.

@SamSaffron

SamSaffron Apr 9, 2013

Contributor

yeah looks good to me. then we can nuke this bit of the change set ... let
me remove it ... once sec

On Tue, Apr 9, 2013 at 12:36 PM, Joshua Peek notifications@github.comwrote:

In lib/sprockets/base.rb:

@@ -228,7 +228,10 @@ def entries(pathname)
#
# Subclasses may cache this method.
def stat(path)

  •  @trail.stat(path)
    
  •  # note, @trail.stat is sub-optimal
    
  •  # it create a new Index in hike, which initiliazed a bunch of
    
  •  # unused stuff, the stat cool in Index disregards root anyway
    
  •  File.stat(path)
    

Check it - sstephenson/hike#23sstephenson/hike#23


Reply to this email directly or view it on GitHubhttps://github.com//pull/428/files#r3706837
.

This comment has been minimized.

@josh

josh Apr 9, 2013

Contributor

Shipped a hike 1.2.2

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 9, 2013

ok @josh per changes to hike I reverted that, PR should be good to merge.

now ... how are we going to get this into rails 3.2.14 with all the gemfile locking drama

.gitignore Outdated
*.rbc
*.swp
*.swo

This comment has been minimized.

@josh

josh Apr 9, 2013

Contributor

Could you keep these to your global ~/.gitignore file.

This comment has been minimized.

@SamSaffron

SamSaffron Apr 9, 2013

Contributor

k will take it out, one sec

On Tue, Apr 9, 2013 at 1:40 PM, Joshua Peek notifications@github.comwrote:

In .gitignore:

\ No newline at end of file
+.rbc
+
.swp
+*.swo

Could you keep these to your global ~/.gitignore file.


Reply to this email directly or view it on GitHubhttps://github.com//pull/428/files#r3707224
.

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Apr 9, 2013

Just needs to be sync'd with master.

@carlosantoniodasilva

This comment has been minimized.

Copy link
Contributor

carlosantoniodasilva commented Apr 9, 2013

@SamSaffron there was only one issue with rails 3.2 as far as I can remember, that if solved would allows us to change the locking.

I think @rafaelfranca and @guilleiguaran can give more background about it, perhaps you can help solving the problem.

Great work here, btw 👍

added a to_i when comparing times on asset, fidelity is already hard
coded to 1 second in lots of other spots
change persistence format to Fixnum from String, upgrade should take
care of cleaning up tmp/cache directory
@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 9, 2013

@josh we should be good to merge now ... did a clean rebase

@SamSaffron

This comment has been minimized.

Copy link
Contributor

SamSaffron commented Apr 9, 2013

@carlosantoniodasilva thanks heaps, would love to help out getting rails on latest sprockets ... now I need to figure out how to serve the 400 or so files that make up discourse faster in dev 😄

josh added a commit that referenced this pull request Apr 9, 2013

Merge pull request #428 from SamSaffron/master
Fix serialization format for mtime on files

@josh josh merged commit 895b53b into sstephenson:master Apr 9, 2013

1 check passed

default The Travis build passed
Details
@carlosantoniodasilva

This comment has been minimized.

Copy link
Contributor

carlosantoniodasilva commented Apr 9, 2013

@SamSaffron 😄👍

@josh

This comment has been minimized.

Copy link
Contributor

josh commented Apr 9, 2013

@SamSaffron ❤️

@josh josh unassigned josh Mar 12, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.