Hi, right, two expressions at different places have a different signature (hash and reference). Even the same expression called twice has a difference signature. This is why when I wrote the Expression comparer I chosed the option to compare the comparable contents. I'm pretty sure it was working at that time. Someone probably added a new comparison, which now causes comparisons to fail.
Do we have simple samples of comparison returning an equality where it should not? Do we also have opposite samples? I don't like the ToString() approach, since it is not proven to be 100% reliable (I'm not sure all sub expressions generate something and something unique where converted to a string). Pascal. On Sat, May 16, 2009 at 13:55, Jonathan Pryor <[email protected]> wrote: > Behold the actual underlying problem to why QueryCache is broken: > > var id = "foo"; > var a = db.Customers.Where(c => c.CustomerID == id); > id = "bar"; > var b = db.Customers.Where(c => c.CustomerID == id); > Console.WriteLine("HashCodes: a={0}; b={1}", a.GetHashCode(), > b.GetHashCode()); > Console.WriteLine("a == b? {0}", a.Equals(b)); > > Output while running under .NET: > > HashCodes: a=38068051; b=17286255 > a == b? False > > In short, it is *not* appropriate to use an expression tree as a key into > a Dictionary, as they do not demonstrate value equality properties. > > QueryCache is broken (and effectively unfixably broken) because the only > useful key into the cache that we have...can't actually be used as a key. > > (Though I suppose we could instead the result of ToString() as the key > into the cache, but more on that below.) > > However, just using ToString() won't fix the unit test breakage I wrote > yesterday. It will instead just make the *existing* (un-patched) > ReadTest.A5_SelectSingleOrDefault() test fail (for precisely the same > "input parameter values are *also* cached" issue I referred to yesterday). > > Finally, I'd like to put try one last effort at pointing out why > CompiledQuery.Compile() is a better approach. Executing a Linq-to-SQL > query takes 3 steps: > > 1. Create the expression tree (done by the compiler and at runtime via > Queryable extension methods). > 2. Convert the expression tree into SQL code (done by DbLinq). > 3. Execute the SQL against the database. > > The current QueryCache approach only caches the result of (2), and if > you've ever profiled expression trees you'll find that (1) can also be quite > slow (often unexpectedly so). (Which is why, while using ToString() would > "fix" the key lookup over using the expression tree itself, still isn't an > ideal fix: ToString() requires traversing the (possibly quite large) > expression tree, and would thus add additional overhead just to perform the > cache lookup.) > > Meanwhile, CompiledQuery.Compile() allows caching *both* steps (1) *and*(2). > There is no need for the delegate returned by > CompiledQuery.Compile() to re-create the expression tree, nor to re-parse > it, which saves time (which is why CompiledQuery exists in the first > place). > > *Please* consider implementing CompiledQuery. > > - Jon > > > On Sat, 2009-05-16 at 09:33 +0200, Giacomo Tesio wrote: > > AFAIK the closures are a sort of "pointer" to the value (actually to a > wrapper of the context that is used). > > > > I suppose that in this case, someone else (like the CLR) would update the > closures values so that the yet compiled query would find the correct values > in the environment. > > > > > > I promise I'll take a look to the QueryCache again monday, but I don't > believe we could actually remove the cache, since the DbLinq's performance > would make it unusable. > > > > The problem here is understanding fully the CLR managing of closures and > understanding why at different time you get different closures. We could > also include the closures in the expression comparisation (which actually > work only on the string rappresentation of the expression itself). > > > > > > Giacomo > > PS: as to the connected datacontext approach: I think that long lived > datacontext should not be "connected", so ours will have > ObjectTrackingEnable == false. > > > > > > On Fri, May 15, 2009 at 7:34 PM, Jonathan Pryor <[email protected]> wrote: > > I think the correct approach is to use CompiledQuery, for precisely the > cache management reasons I originally outlined. > > Though long-lived DataContexts would be problematic, as you point out, as > you'd effectively clone the entire DB into memory given enough time and > queries... I don't see how a connected database approach (which > DataContext follows) could behave otherwise. > > Does DbLinq support compiled queries? find says: > > $ find src -name CompiledQuery.cs > src/DbLinq/System.Data.Linq/CompiledQuery.cs > > Survey says...No. I believe *we should*. > > I'm not sure what you mean by our queries being static. Once we cache a > query, we don't change it anymore. (If we did, we'd be altering the > original query, which would be...bad.) > > I'm also not sure what you're getting at with closures. > > The way I envision CompiledQuery working is very similar/identical to how > your existing query caching works, except instead of storing the > SelectQuery type within QueryCache, it would instead be stored within the > delegate returned from CompiledQuery.Compile(). Thus, the user actually > deals with pre-parsed query statements, and DbLinq doesn't need to re-parse > the LINQ statement again (as the delegate returned by > CompiledQuery.Compile() stores the pre-parsed SelectQuery instance). > > I'm not sure how much work this would take, but I'm hopeful that it > wouldn't be too much work. > > *However*, this does bring up two related issues. > > 1. I still need to figure out wtf is going on with the NerdDinner caching > bug I was seeing last week (and better, how to reproduce in the unit tests > so that you can take a better look at it). > > 2. SelectQuery.GetCommand() gives me *really* bad feelings, because: > > 1. It takes no arguments. > 2. It calls InputParameterExpression.GetValue() with no parameters. > 3. (A) and (B) together imply that, even though the underlying SELECTtakes > named parameters (yay), there's no way to actually provide them/alter > them for the current SelectQuery instance (wtf?). > > I *think* this is why (1) fails for me (but again, I still need to debug). > > In any event, it makes no sense to me at all. The point to having a > SELECT with parameters is so that you can cache the expression itself but > vary the parameters. But since we're not providing any parameters, the > parameters can't vary. So... > > It makes my head hurt, if nothing else. > > I would instead expect AbstractQuery.GetCommand() to take an 'object[] > parameters' argument (or similar) so that we can cache the actual select > statement w/o associated parameters. This would also dovetail nicely with > the semantics CompiledQuery.Compile(), as you can provide parameters to > the expression you're compiling: > > var pepleWithLastName = CompiledQuery.Compile( > (PeopleDb db, string lastName, int start, int count) => > (from p in db.People > where p.LastName == lastName > select p) > .Skip(start) > .Take(count)); > foreach (p in peopleWithLastName(myDB, "Foo", 0, 1)) ... > > Alas, CompiledQuery.Compile() will only create delegates accepting 4 > parameters, but there are workarounds... > - Jon > > > > > On Fri, 2009-05-15 at 16:35 +0200, Giacomo Tesio wrote: > > I'll really hope is it right... :-| > It would be a great problem otherwise. > > AFAIK, the datacontext rappresent a UnitOfWork. If the unit of work is long > as the application lifetime, than keeping it alive is right. > But in an internet application delivered via http, I've got my doubt. > > If it's not readonly, the tracked entities would become an unaligned copy > of the full database... in memory. > > > That said, does DbLinq support compiled queries? > > The problem with this approach, would be that our queries are not static: > we progressively add clausoles to IQueryable<T>. > And, how to handle with closures? > > > Giacomo > > > > On Fri, May 15, 2009 at 1:37 PM, Jonathan Pryor <[email protected]> wrote: > > I think you'll find that you're doing it wrong. :-) > > First, I'm not sure that the assertion that all apps have short-lived > DataContexts is correct. That's certainly not the case for NerdDinner, > which has only one DataContext for the lifetime of the app. Many apps may > have short-lived DataContexts, but many won't. > > Secondly, and primarily, Microsoft doesn't do implicit query caching. They > do *explicit* query caching: > > > http://blogs.msdn.com/ricom/archive/2008/01/11/performance-quiz-13-linq-to-sql-compiled-queries-cost.aspx > > http://blogs.msdn.com/ricom/archive/2008/01/14/performance-quiz-13-linq-to-sql-compiled-query-cost-solution.aspx > > For example: > > var fq = CompiledQuery.Compile > ( > (Northwinds nw) => > (from o in nw.Orders > select new > { > OrderID = o.OrderID, > CustomerID = o.CustomerID, > EmployeeID = o.EmployeeID, > ShippedDate = o.ShippedDate > }).Take(5) > ); > > The result of CompiledQuery.Compile is a pre-compiled, pre-analyzed > query, which the *user* is responsible for caching and dealing with. > Query caches are *not* part of DataContext itself, *precisely because*it's a > recipe for a giant memory leak. > > (Consider a fictional app which uses Linq-to-SQL once at startup, or > otherwise very infrequently. The DbLinq approach would assure that the > original cached queries would never be freed; it would be a permanent memory > tax on the app.) > > I would *strongly *suggest that you follow Microsoft's approach, drop the > DataContext query caching, and use CompiledQuery instead. > > - Jon > > > > On Fri, 2009-05-15 at 10:25 +0200, Giacomo Tesio wrote: > > OFFTOPIC about the interfaces > IQueryCache is an example of an interface which must not be removed. This > way if a skilled programmer would really need it, it could reimplement it to > better fill it's requirement. > > Even if using ReaderWriterLockSlim, thread safety has a cost: some one > could require speed rather than thread safety. > Than it should be possible to reimplement it. > > Another use of such an interface could be to move the cache out of the > appdomain (in dedicated cache servers) and share them among servers. This > would also make the cache livecicle longher than the iis application. > I'm not a fan of this solution (I'm not sure this would lead in better > performances), but in an enteprise environment like ours, it had to be > possible to take such a decision later. > > That's why I think that good interfaces are a good thing: we should not > decide how DbLinq as to be used (tecnologically speaking of course). > > Having GOOD internal interfaces allow better flexibility in the long run, > and produce a better open source product. > > > BAD interfaces, on the other hand, reduce flexibility and improve > developments effort, but I think that they always underling a wrong analisis > or design (if not a completely missing one). > > > I encounter often .NET programmers talking against interfaces. But till > now, I've always noticed they are talking about wrong interfaces they have > designed bottom up. It could took much time to explain a Microsoft .NET > developer that the problem are not the interfaces, the problem is their > design. > > More or less like explaing them that a Domain Model IS Object Orientation, > not a way of doing Object Orientation. > Or to explain them that a object oriented language does not lead by itself > to object oriented software... > > > Ok... I don't like Microsoft. :-D > > > > Giacomo > > > On Fri, May 15, 2009 at 10:00 AM, Giacomo Tesio <[email protected]> wrote: > > That's a good question, i think. > > QueryCache is a cache of generated queries for each expression tree > evaluated. > It actually has to be static (at least thread static, but this would > multiply the memory usage per number of threads, also reducing the hits and > reducing livetime to the thread one) to improve the hits, since in the most > common DataContext use case, it rappresents a unit of work and have short > live. > If the QueryCache livecicle would match the DataContext one, probably it > would have no reason to exists. > > In "our" DbLinq use case, a readonly DataContext is used from all threads > and has a long live (the AppDomain one), while single unit of works actually > are created per request or on a per need basis. > > The global readonly one, has to be no "instance caches" at all (no object > tracking for example, and I hope there are no other caches... but actually I > should indagate this more), but still need a QueryCache becouse it could > share its yet parsed expression tree among threads. > > Moreover all the DbLinq DataContexts would benefit from such a static > queries, greatly increasing the performances (I hope! ! ! :-D). > > Other caches (like the EntityTrakings one) must not be static since they > are conceptually linked to the DataContext that fill and use them. > > > > > Giacomo > > > > > On Thu, May 14, 2009 at 5:19 PM, Jonathan Pryor <[email protected]> wrote: > > This is bound to be a stupid/silly question, but why do the caches need to > be static? Static data is effectively global, i.e. a GC root in and of > itself, and thus will never be collected. Even with a good policy, it's > possible that this could use more memory than people would expect. > > See also: > > http://blogs.msdn.com/oldnewthing/archive/2006/05/02/588350.aspx<http://blogs.msdn.com/oldnewthing/archive/2006/05/02/588350.aspx> > http://blogs.msdn.com/ricom/archive/2004/01/19/60280.aspx > > Is there *really* a need for a cache that's static (i.e. shared amongst > all DataContext instances)? Or can it just be non-static and attached to > the DataContext (which would also remove all thread safety requirements). > > Put another way, with non-shared caches if the DataContext gets collected > then the cache is also collected, thus providing a natural mechanism to > clear the cache. With shared (static) caches, they're not connected to > the DataContext, and thus it could be holding cached data for a > DataContext that no longer exists. (This may not be the case anyway; I > haven't fully read and understood the code. I'm just trying to make clear > that preferring shared caches isn't an open and shut easy decision.) > > Thanks, > - Jon > > > > On Thu, 2009-05-14 at 16:55 +0200, Giacomo Tesio wrote: > > I've two need: > - Thread Safety of static caches: should be done for QueryCache, but Jon > have encountered a strange (unreproduced on tests) bug while working on > NerdDinner. If no other static cache exists they are ok. > > > - XmlMappingSource working correctly: now, associations are not loaded from > external mappings. This fix require IDataMapper and DataMapper modifications > and DataContext fixes. > > > The first is absolutelly needed (but should yet work right, just missing > true multithread test on multi core machines). > The second I think is really important, but require a bit of work. > > > > Giacomo > > > > On Thu, May 14, 2009 at 4:28 PM, Sharique <[email protected]> wrote: > > > Hi, > It has been almost 1 year since last release, I think it is time make > a new release (guess 0.19) . If there is any blocking issue, pls put > it here for discussion. So that we can resolve it quickly. > > -- > Sharique > > > > > > > > > > > > > > > > > > > > > > > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "DbLinq" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/dblinq?hl=en -~----------~----~----~----~------~----~------~--~---
