Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Razor caching to drastically improve performance. #202

Merged
merged 6 commits into from
Sep 29, 2014

Conversation

matthid
Copy link
Member

@matthid matthid commented Sep 29, 2014

The speed of the API reference generation (and possibly some other things) can be improved drastically by using razor caching (https://razorengine.codeplex.com/wikipage?title=Quick%20Start%20Guide).

In my case I could reduce the time to produce the docs from 600 to 21 secs!
The first two commits improve the error messages (second is a bugfix).

Maybe you should not merge this patch and instead rewrite/remove the RazorRender class to only use caching (ie. currently the class is designed around the ProcessFile function, and my extensions are currently a bit error prone), and do the generation it in parallel again.

Note: The ProcessDirectory function in FSharp.Literate could also benefit from this when ProcessFileCache is used (and CompileTemplate is called at least once).

@forki
Copy link
Member

forki commented Sep 29, 2014

Wow this sounds like a massive improvement.
Since I'm only temporarily maintainer I don't think we should rewrite. But using cache seems like something I could merge

@forki
Copy link
Member

forki commented Sep 29, 2014

Btw AFAIK the long term strategy is to get rid of razor if possible.

@ovatsus
Copy link

ovatsus commented Sep 29, 2014

Even if eventually want to get rid of razor I would merge and publish a new version with this. It's a great improvement

@forki
Copy link
Member

forki commented Sep 29, 2014

Yes that's what I think, too. Will look at it and try to release.
On Sep 29, 2014 1:32 PM, "Gustavo Guerra" notifications@github.com wrote:

Even if eventually want to get rid of razor I would merge and publish a
new version with this. It's a great improvement


Reply to this email directly or view it on GitHub
#202 (comment)
.

@matthid
Copy link
Member Author

matthid commented Sep 29, 2014

Why get rid of razor? I did like it, and it is actually very fast :)

I'm not sure why the build would fail, because I use custom solution files (removed paket) and custom deps (custom RazorEngine that works on mono). It is hard for me to run the unit tests locally. But if you look into it I guess I can ignore it for now?

@matthid
Copy link
Member Author

matthid commented Sep 29, 2014

Sorry it seems like I was a little bit too fast :(.

@forki
Copy link
Member

forki commented Sep 29, 2014

no problem. Just tell me when it's ready to merge

File.WriteAllText(outDir @@ (typ.UrlName + ".html"), out)
Log.logf "Finished type: %s" typ.UrlName
razor)
//Parallel.pfor types (fun () -> RazorRender(layoutRoots, ["FSharp.MetadataFormat"])) (fun typ _ razor ->
// Log.logf "Generating type: %s" typ.UrlName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please remove this dead code. thanks

@matthid
Copy link
Member Author

matthid commented Sep 29, 2014

The build is still failing because I used a newer version of RazorEngine, I don't know if the latest nuget version is enough, but could you try updating RazorEngine to the latest nuget version anyway?

@forki
Copy link
Member

forki commented Sep 29, 2014

try the following:

@matthid
Copy link
Member Author

matthid commented Sep 29, 2014

I tried that, but I was not able to make it work. I think the new versions are only available for net4.5 and this solution is still 4.0.

But this is an issue for another time, I have added a (rather dirty) workaround for now.

The rewriting of the RazorRender is now also done, so I guess this can be tested/merged? However to leverage this speed for *.md files, one would have to rewrite the ProcessDirectory logic in a way that it reuses a RazorRender instance. I don't need this as I have only a few of those.

Note: It is now actually even faster than written in the first post (15 secs).

@forki
Copy link
Member

forki commented Sep 29, 2014

Oh right. They moved to 4.5

@matthid
Copy link
Member Author

matthid commented Sep 29, 2014

Is there a discussion/faq why you want to move away from razor? Razor seems like a perfect fit.

@forki forki merged commit ef989da into fsprojects:master Sep 29, 2014
@forki
Copy link
Member

forki commented Sep 29, 2014

advert

thanks to the support by @jeffhandley it's released in 2.4.26. awesome!

@forki
Copy link
Member

forki commented Sep 29, 2014

why you want to move away from razor?

AFAIK mostly because of xplat. We don't get it to work in the downstream projects.
#188

@forki
Copy link
Member

forki commented Sep 29, 2014

already using it in Paket - see fsprojects/Paket@1e824af

time for docs form 18 seconds down to 4. That's awesome.

Now the 10s for the md files seems like way too much ;-)

@matthid
Copy link
Member Author

matthid commented Sep 29, 2014

Yeah I'm using this on mono actually...
But I had to bundle my own RazorEngine.dll, which is a bit of a pain.

@forki
Copy link
Member

forki commented Sep 29, 2014

oh that's interesting.
we could replace the nuget package and just bundle your build.

@matthid
Copy link
Member Author

matthid commented Sep 29, 2014

See matthid/RazorEngine@2bebf75 for details.

Basically what I do is use this static field to replace the references completely with my own list:

if isMono then
    // Workaround compiler errors in Razor-ViewEngine
    DirectCompilerServiceBase.EditLoadedAssemblyList <- new System.Func<IEnumerable<string>, IEnumerable<string>>(fun loadedList ->
        // We replace the list and add required items manually as mcs doesn't like duplicates...
        let getItem name =
            loadedList |> Seq.find (fun l -> l.Contains name)
        [ (if isMono then "/usr/lib64/mono/gac/FSharp.Core/4.3.1.0__b03f5f7f11d50a3a/FSharp.Core.dll" else "FSharp.Core") 
          getItem "FSharp.Compiler.Service"
          Path.GetFullPath @".nuget/Build/Microsoft.AspNet.Razor/lib/net45/System.Web.Razor.dll"
          getItem "RazorEngine"
          getItem "FSharp.Literate"
          getItem "FSharp.CodeFormat"
          getItem "FSharp.MetadataFormat"
           ] |> List.toSeq
        )

@forki
Copy link
Member

forki commented Sep 29, 2014

do you see any chance to get this back into razor?
(and to get the team release a 4.0 version?)

@matthid
Copy link
Member Author

matthid commented Sep 29, 2014

I could try, but I don't think they would merge these changes, that's definitely not the way it's supposed to be done :).

@forki
Copy link
Member

forki commented Sep 29, 2014

true but it's an outdated package and they do not really care about this anymore, right?

I mean they don't support 4.0 any more.

@forki
Copy link
Member

forki commented Sep 30, 2014

After thinking about this:
I think we should bundle your dll and remove the nuget dependency.

@matthid
Copy link
Member Author

matthid commented Sep 30, 2014

Some notes to my patches:
Watch out that the current RazorRender type is a bit weird (in fact you can use the same instance in multiple threads but not multiple instances at the same time!), I think the best way to fix this is to call the methods on the TemplateService instance directly instead of using the static Razor type. Maybe I submit another pull request when I find some time for it.

This will most likely introduce a whole range of bugs when someone tries to generate API docs and *.md files in parallel.

@forki
Copy link
Member

forki commented Sep 30, 2014

Maybe I submit another pull request when I find some time for it.

that would be awesome ;-)

2014-09-30 14:02 GMT+02:00 matthid notifications@github.com:

Some notes to my patches:
Watch out that the current RazorRender type is a bit weird (in fact you
can use the same instance in multiple threads but not multiple instances at
the same time!), I think the best way to fix this is to call the methods on
the TemplateService instance directly instead of using the static Razor
type. Maybe I submit another pull request when I find some time for it.

This will most likely introduce a whole range of bugs when someone tries
to generate API docs and *.md files in parallel.


Reply to this email directly or view it on GitHub
#202 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants