-
Notifications
You must be signed in to change notification settings - Fork 97
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
Server-side caching of viewmodels #704
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments about the server side, I'll go though the JS part later.
In general, I like the idea of this mechanism, it may improve the bandwidth requirements quite significantly. However, especially on s***ty connection the chaining of requests in case the VM is not on the server may cause issues (when combined with #705 there may be 3 requests). When there is a fixed timeout of few minutes, I think we could also explain that to the client, so it does not even try to send the diff.
src/DotVVM.Framework/ViewModel/Serialization/DefaultViewModelServerCache.cs
Outdated
Show resolved
Hide resolved
src/DotVVM.Framework/ViewModel/Serialization/DefaultViewModelServerCache.cs
Outdated
Show resolved
Hide resolved
src/DotVVM.Framework/ViewModel/Serialization/DefaultViewModelServerCache.cs
Show resolved
Hide resolved
src/DotVVM.Framework/ViewModel/Serialization/IViewModelServerStore.cs
Outdated
Show resolved
Hide resolved
{ | ||
private readonly IDotvvmCacheAdapter cacheAdapter; | ||
|
||
public TimeSpan CacheLifetime { get; set; } = TimeSpan.FromMinutes(5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite short IMHO and it's not trivial to reconfigure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would however make sense to somehow remove entries that were used by client that got a new model from the server. For that we'd have to pass some client identifier to the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lifetime management will change definitely, I need to do some measurements. There should also be some limits per route. The problem with removing cache entries is that you don't know how many people are using them. We'd need to add some reference counting - maybe the short lifetime will work well enough.
The lifetime management will change definitely, I need to do some
measurements. There should also be some limits per route. The problem
with removing cache entries is that you don't know how many people are
using them. We'd need to add some reference counting - maybe the short
lifetime will work well enough.
It's true I also have no idea how will people use, but I also don't know
how to measure that. I'd also try to store the viewmodels for infinite
time (or maybe a day or so) and low priority and let the ASP.NET cache
handle the pressure.
Also, if we'd a decent global compression mechanism, the cost of one
view model could get very low, most of the large things are going to be
very repetitive anyway. It would be very intereting to have a look at a
site with significant traffic and try to come up with something, if
you'd know of something I'd be interested to have a look.
|
It's not thread-safe for sure, see
<https://source.dot.net/#System.Security.Cryptography.Primitives/System/Security/Cryptography/HashAlgorithm.cs,106>.
The `ComputeHash` method accumulates some hash on itself and then
returns it. If multiple threads do that, it will certainly break.
|
Newtonsoft.Json has support for BSON, but in 12.0 and older versions
it's included in the Newtonsoft.Json package itself. From 12.0 it is in
the separate package. Since DotVVM tries to request the lowest
Newtonsoft.Json as possible, it will be difficult to make this binary
compatible. Maybe we can use Reflection to find the BsonWriter and
BsonReader class.
Ok, thats a bit too complicated, let's not care about for now. BSON does
not bring that big improvement anyway.
|
And btw ReadOnlyMemory is only in .NET Standard 2.1 which means we
cannot use this for .NET Framework. I will use byte array and BSON - it
should be more efficient than parsing and writing the string.
This package https://www.nuget.org/packages/System.Memory should work
with almost anything. We don't need the native support from the runtime.
|
What will be the benefit of ReadOnlyMemory over byte[]? I found that the lowest version of Newtonsoft.Json.Bson supports 10.0.3 which is minimum version of Newtonsoft.Json required by DotVVM, so it should not be an issue. I'll continue working on this later.
I don't know yet if we want to have some auto-tuning in the framework that would decide whether the cache is good or not, or if we just give the user the tools to decide on their own. I would definitely start with the second way, but I am not sure if anyone will use it if it requires some work to set it up. |
string viewModelCacheId = null; | ||
if (context.Configuration.ExperimentalFeatures.ServerSideViewModelCache.IsEnabledForRoute(context.Route.RouteName)) | ||
{ | ||
viewModelCacheId = viewModelServerCache.StoreViewModel(context, (JObject)viewModelToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing, we should only store properties that sent to client and also back to server. This way we are wasting quite a bit of space. Fortunately, the serializer should silently ignore properties that should not be sent to server, so there is no change in behavior.
Unless you use the Direction.ClientToServerNotInPostbackPath
, then the serializer will take it into account even though it should not be sent at all (assuming it was not in the path). Unfortunately, these properties can't be just dropped as they might also be needed when they are in the path. And, on the server side, we have basically no way of knowing which object are in the path during the serialization phase, so I don't see a simple fix to that :/ Maybe we could figure out the JSON path on client and send it to the server.
TBD:
Then we can merge. |
Js and C# tests for JSON diff & patch implemented
@@ -399,6 +399,10 @@ public WriterDelegate CreateWriterFactory() | |||
{ | |||
options["pathOnly"] = true; | |||
} | |||
if (!property.TransferAfterPostback) | |||
{ | |||
options["firstRequest"] = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fan of transmitting all those options to the client. It's already annoyingly large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know and we should definitely de-duplicate them and put them in the $type definitions together with the validation rules. But I'd rather solve this in a separate PR.
Disclaimer: This is an experimental feature - it should not be merged in master any time soon without extensive testing.
I have implemented a relatively simple mechanism that can dramatically decrease the amount of data transferred between the client and the server on postbacks. Basically, the viewmodel is cached on the server and its hash is used as the cache key (that should keep the cache small for static pages or for pages with identical initial state).
viewModelCacheId
field is included to the serialized viewmodel.dotvvm.viewModels['root'].viewModelCache
when the page is loaded.viewModelDiff
andviewModelCacheId
instead of the full viewmodel.viewModelNotCached
notice and the client repeats the postback with the full viewmodel.Notes
The solution is backwards-compatible - the client may decide to always send the full viewmodel and server must support it.
We should make this feature optional and allow to turn it only for individual pages.
Also, a security review of this feature should be made.The CSRF token and encrypted values are not part of the cache mechanism and are sent (and verified) on all requests as it was before.
The viewmodel serialization and deserialization are using synchronous API right now. The cache may require using an async API (in case the user would like to use some distributed cache etc.) and maybe the serialization itself could benefit from async as well.
And finally, I haven't done any performance comparisons yet - I assume that hashing and caching the viewmodels should be way faster than transferring unnecessary data, however we should measure the impacts on a real-world application.