SmallInetger as methods was not ready yet....now maybe?
Locked
12
12
SmallInetger as methods was not ready yet....now maybe?
 
|
Hi folks. After fighting and fighting about crashes using SmallInteger as methods, I finally could debug the VM (thanks Esteban for the help in compiling and debugging!!) and I think I found a problem in theGC. In the mark phase of the GC, it tries to mark all interpreter oops in the method: markAndTraceInterpreterOops If you see that method....it does this (a part of it): compilerInitialized ifTrue: [self markAndTrace: receiver. self markAndTrace: method] ifFalse: [self markAndTrace: activeContext]. self markAndTrace: messageSelector. self markAndTrace: newMethod. If you are using SmallInteger as methods.....newMethod can be a SmallIneteger, and not a method....so if we then see the method markAndTrace: the first lines are: | header lastFieldOffset action statMarkCountLocal | header := self longAt: oop. And of course, it crash in that #longAt: :) So, solutions: 1) Put an if in each place where it uses newMethod or method or newNativeMethod or suspendedMethods or whatever 2) Put an if in #markAndSweep. I think 2) is easier and it is just adding one line of code at the beginning: | header lastFieldOffset action statMarkCountLocal | (self isIntegerObject: oop) ifTrue: [ ^ 0 ]. header := self longAt: oop. .... what do you think ? Finally, I am afraid that there are more places where Interpreter uses any of those instVar that represent methods, and treat them as real objects. So maybe there still pending future possible crashes? Thanks Mariano |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
On Fri, Dec 3, 2010 at 3:41 PM, Mariano Martinez Peck <[hidden email]> wrote: Hi folks. After fighting and fighting about crashes using SmallInteger as methods, I finally could debug the VM (thanks Esteban for the help in compiling and debugging!!) and I think I found a problem in theGC. For example, take a look to the methods: printUnbalancedStackFromNamedPrimitive primitiveMethod internalJustActivateNewMethod internalActivateNewMethod activateNewMethod And all the senders of literal: offset ofMethod: methodPointer literalCountOf: methodPointer primitiveIndexOf: maybe supporting SmallInteger as methods was not a good idea :( cheers mariano Thanks |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
On 3 December 2010 16:58, Mariano Martinez Peck <[hidden email]> wrote: > > > > On Fri, Dec 3, 2010 at 3:41 PM, Mariano Martinez Peck <[hidden email]> wrote: >> >> Hi folks. After fighting and fighting about crashes using SmallInteger as methods, I finally could debug the VM (thanks Esteban for the help in compiling and debugging!!) and I think I found a problem in theGC. >> In the mark phase of the GC, it tries to mark all interpreter oops in the method: markAndTraceInterpreterOops >> >> If you see that method....it does this (a part of it): >> >> compilerInitialized >> ifTrue: [self markAndTrace: receiver. >> self markAndTrace: method] >> ifFalse: [self markAndTrace: activeContext]. >> self markAndTrace: messageSelector. >> self markAndTrace: newMethod. >> >> If you are using SmallInteger as methods.....newMethod can be a SmallIneteger, and not a method....so if we then see the method markAndTrace: >> >> the first lines are: >> >> | header lastFieldOffset action statMarkCountLocal | >> header := self longAt: oop. >> >> >> And of course, it crash in that #longAt: >> >> :) >> >> So, solutions: >> >> 1) Put an if in each place where it uses newMethod or method or newNativeMethod or suspendedMethods or whatever >> 2) Put an if in #markAndSweep. >> >> I think 2) is easier and it is just adding one line of code at the beginning: >> >> | header lastFieldOffset action statMarkCountLocal | >> (self isIntegerObject: oop) ifTrue: [ ^ 0 ]. >> header := self longAt: oop. >> .... >> >> what do you think ? >> >> >> Finally, I am afraid that there are more places where Interpreter uses any of those instVar that represent methods, and treat them as real objects. >> So maybe there still pending future possible crashes? >> > > > For example, take a look to the methods: > > printUnbalancedStackFromNamedPrimitive > primitiveMethod > internalJustActivateNewMethod > internalActivateNewMethod > activateNewMethod > > And all the senders of > > literal: offset ofMethod: methodPointer > literalCountOf: methodPointer > primitiveIndexOf: > determined that given oop is a compiled method, not a smallinteger or something else. > maybe supporting SmallInteger as methods was not a good idea :( > it should work. > cheers > > mariano > > >> >> Thanks >> >> Mariano > > > -- Best regards, Igor Stasenko AKA sig. |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
On Fri, Dec 3, 2010 at 6:42 PM, Igor Stasenko <[hidden email]> wrote:
But they do not. Let's take an example: literal: offset ofMethod: methodPointer sender: methodClassOf: methodPointer sender: superclassSend sender: doubleExtendedDoAnythingBytecode and singleExtendedSuperBytecode did I miss something?
|
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by Mariano Martinez Peck
> maybe supporting SmallInteger as methods was not a good idea :( FWIW, Objects as methods weren't intended for SmallInts; they were intended for stuff like method wrappers. Using SmallIntegers for methods is a hack and I'm not surprised it doesn't work. But it is easy to fix - just wrap your ints into something else and you're good to go. Cheers, - Andreas |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
On 3 December 2010 19:02, Andreas Raab <[hidden email]> wrote: > >> maybe supporting SmallInteger as methods was not a good idea :( > > FWIW, Objects as methods weren't intended for SmallInts; they were intended > for stuff like method wrappers. Using SmallIntegers for methods is a hack > and I'm not surprised it doesn't work. But it is easy to fix - just wrap > your ints into something else and you're good to go. clearly, VM should not crash if it finds smallint in method dictionary. Of course, it may 'not work', but in less destructive way. > > Cheers, > - Andreas > -- Best regards, Igor Stasenko AKA sig. |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
On 12/3/2010 10:07 AM, Igor Stasenko wrote: > On 3 December 2010 19:02, Andreas Raab<[hidden email]> wrote: >>> maybe supporting SmallInteger as methods was not a good idea :( >> FWIW, Objects as methods weren't intended for SmallInts; they were intended >> for stuff like method wrappers. Using SmallIntegers for methods is a hack >> and I'm not surprised it doesn't work. But it is easy to fix - just wrap >> your ints into something else and you're good to go. > > clearly, VM should not crash if it finds smallint in method dictionary. > Of course, it may 'not work', but in less destructive way. Well, there are limits. By the same argument you can come up with any number of things that currently crash the VM where one might say the VM should react in a less destructive way (thisContext swapSender: 0 anyone?). Performance is important in this area and having to throw in a bunch of checks all over the place is something that I'm not to fond of. If you really want to do something here I'd do it entirely differently: Add a primitive to the VM that can answer the question "can you execute this object?" and use it before adding something to the method dictionary. That way you won't be able to create a method dictionary that contains stuff which is not executable. Cheers, - Andreas |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
On 3 December 2010 19:15, Andreas Raab <[hidden email]> wrote: > > On 12/3/2010 10:07 AM, Igor Stasenko wrote: >> >> On 3 December 2010 19:02, Andreas Raab<[hidden email]> wrote: >>>> >>>> maybe supporting SmallInteger as methods was not a good idea :( >>> >>> FWIW, Objects as methods weren't intended for SmallInts; they were >>> intended >>> for stuff like method wrappers. Using SmallIntegers for methods is a hack >>> and I'm not surprised it doesn't work. But it is easy to fix - just wrap >>> your ints into something else and you're good to go. >> >> clearly, VM should not crash if it finds smallint in method dictionary. >> Of course, it may 'not work', but in less destructive way. > > Well, there are limits. By the same argument you can come up with any number > of things that currently crash the VM where one might say the VM should > react in a less destructive way (thisContext swapSender: 0 anyone?). > Performance is important in this area and having to throw in a bunch of > checks all over the place is something that I'm not to fond of. > > If you really want to do something here I'd do it entirely differently: Add > a primitive to the VM that can answer the question "can you execute this > object?" and use it before adding something to the method dictionary. That > way you won't be able to create a method dictionary that contains stuff > which is not executable. > method dictionary which contains 'non-executable' objects. methodDict at: #foo put: foo. foo becomeForward: somethingNonExecutable. so, adding a primitive which tells if given object could be added to method dictionary or not gives nothing in a sense of having VM which doesn't crash at some point. > Cheers, > - Andreas > -- Best regards, Igor Stasenko AKA sig. |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
On 12/3/2010 10:33 AM, Igor Stasenko wrote: > the problem is, that when you want it, you can always end up with > method dictionary which contains 'non-executable' objects. > > methodDict at: #foo put: foo. > foo becomeForward: somethingNonExecutable. So don't do that! When was the last time you've had the need to #become: CompiledMethods anyway? ;-) Oh, and for the terminally curious, try evaluating the following just for fun: Object new becomeForward: 3. See? ;-) Cheers, - Andreas |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by Andreas.Raab
On Dec 3, 2010, at 7:02 PM, Andreas Raab wrote: >> maybe supporting SmallInteger as methods was not a good idea :( > > FWIW, Objects as methods weren't intended for SmallInts; they were intended for stuff like method wrappers. Using SmallIntegers for methods is a hack and I'm not surprised it doesn't work. Me too. They were worked well for normal objects. This is good and valuable hook. > But it is easy to fix - just wrap your ints into something else and you're good to go. It does not work in mariano special need because the idea was to push as far as possible the idea that we do not have to allocate any new objects and avoid any proxy memory allocation cost. So clearly this is may be a too hackish hack. Else a plain simple (but allocating memory proxy works well but we wanted to know what is the lowest boundary). > > Cheers, > - Andreas |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by Mariano Martinez Peck
> > > > no, but these methods usually should be fired only after you already > determined that given oop is a compiled method, > not a smallinteger or something else. > > > But they do not. Let's take an example: literal: offset ofMethod: methodPointer > sender: methodClassOf: methodPointer > sender: superclassSend > sender: doubleExtendedDoAnythingBytecode and singleExtendedSuperBytecode > > did I miss something? I have the impression that it shows that SmallIntegers are not really the same as any other objects :) So may be you should evaluate if you want to patch all the places to see how much you lose in speed and you get your lower bound. But may be this can be tedious. Stef |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by Andreas.Raab
Andreas I understand your point. Now this is a bit 'sad' (I do not know how to express it) because it shows somehow that SmallIntegers are not uniformly treated as normal objects (of course they are not from a VM perspective) but this illusion on the image side is so great that having it on the vm would be cool. Mariano I would be curious to know the cost of your check on a real execution and also with cog to get an idea. Now from a robustness (availability point of view) it would be good that for things that are not in the main execution loop checks are done because they would increase the general robustness of the system. This was the idea of the changeClassToThatOf: design (forcing the dev to produce an instance of class and not specifying the class) to make sure that the clss to which the instance was change to was a class that could get instances. Stef > On 12/3/2010 10:33 AM, Igor Stasenko wrote: >> the problem is, that when you want it, you can always end up with >> method dictionary which contains 'non-executable' objects. >> >> methodDict at: #foo put: foo. >> foo becomeForward: somethingNonExecutable. > > So don't do that! When was the last time you've had the need to #become: CompiledMethods anyway? ;-) Oh, and for the terminally curious, try evaluating the following just for fun: > > Object new becomeForward: 3. > > See? ;-) > > Cheers, > - Andreas > |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by stephane ducasse-2
On Fri, 3 Dec 2010, stephane ducasse wrote: > > > On Dec 3, 2010, at 7:02 PM, Andreas Raab wrote: > >>> maybe supporting SmallInteger as methods was not a good idea :( >> >> FWIW, Objects as methods weren't intended for SmallInts; they were intended for stuff like method wrappers. Using SmallIntegers for methods is a hack and I'm not surprised it doesn't work. > > Me too. They were worked well for normal objects. This is good and valuable hook. > >> But it is easy to fix - just wrap your ints into something else and you're good to go. > > It does not work in mariano special need because the idea was to push as far as possible the idea that we do not have to allocate any new objects and avoid any proxy memory allocation cost. So clearly this is may be a too hackish hack. Else a plain simple (but allocating memory proxy works well but we wanted to know what is the lowest boundary). I have a working package that can swap out CompiledMethods and replace them with SmallIntegers in the MethodDictionaries. When the method is about to be used by the system, the CompiledMethod is automatically swapped back. It uses the existing ObjectsAsMethods implementation, so I say that SmallIntegers can be used like any other object in this regard. I didn't test it with SqueakVM, but it definitely works with CogVM. Levente > >> >> Cheers, >> - Andreas > > |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by Mariano Martinez Peck
Hi Mariano, On Fri, Dec 3, 2010 at 6:41 AM, Mariano Martinez Peck <[hidden email]> wrote:
Either. But if you go with 1) you should put an assert in markAndSweep:. The current VM style seems to be to go with 1, although I agree that 2 is the better approach.
Finally, I am afraid that there are more places where Interpreter uses any of those instVar that represent methods, and treat them as real objects. We live and learn :)
Welcome to the vm community. You're doing it now!! best Eliot |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by Andreas.Raab
On Fri, Dec 3, 2010 at 10:15 AM, Andreas Raab <[hidden email]> wrote:
Well, if the vm supports "objects as methods" then there needs to be a check for something fetched from a method dictionary being a CompiledMethod somewhere. It is easy to make this robust in the face of SmallIntegers. The elegance is in writing the code so that the check occurs in as few places as possible. In my current Cog I've managed to get this down to two places in the normal lookup machinery, addNewMethodToCache: for the interpreter and lookupAndCog:for: for inline cache misses. Primitives like primitiveDoNamedPrimitiveWithArgs and primitiveExecuteMethodArgsArray need to check also (5 in all in Cog) but for the normal lookup machinery things feel OK.
BTW, Mariano's use of SmallIntegers rather than wrappers is an attempt to get a very compact image and seems a reasonable experiment. Using wrappers would defeat his use-as-little-memory-as-possible purpose.
best Eliot
|
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
On 12/3/2010 4:18 PM, Eliot Miranda wrote: > BTW, Mariano's use of SmallIntegers rather than wrappers is an attempt > to get a very compact image and seems a reasonable experiment. Using > wrappers would defeat his use-as-little-memory-as-possible purpose. Not at all. The design of objects-as-methods was deliberately done such that one could use a single wrapper for multiple methods. The only thing you need is to map from selector to method being executed. For example: MethodSwapper>>run: aSelector with: argsArray in: aReceiver "Execute the given method" method:= methodMap at: aSelector. ^aReceiver withArgs: argsArray executeMethod: method Then you install a single MethodSwapper instance like here: swapper := MethodSwapper new. Morph selectorsAndMethodsDo:[:sel :meth| swapper methodMap at: sel put: meth]. Morph selectorsDo:[:sel| Morph addSelector: sel withMethod: swapper]. At this point the only overhead you have is what's in the (single) MethodSwapper instance and even with a most naive implementation (looking up the index by selector) you'd have 8 bytes per method in your image which I suspect is well below 5% of the image size. And then of course you can start *really* looking at getting the size down. Did you know that Dan once did a very neat hack where he replaced all symbols with integers? Works great and if you do that you not only reclaim the space for the symbols but also don't need a lookup at all :-) Cheers, - Andreas |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
On Fri, Dec 3, 2010 at 5:21 PM, Andreas Raab <[hidden email]> wrote:
Ah, much nicer. Then you install a single MethodSwapper instance like here: I didn't know that Dan had done this. ActiveBook did this with the Smalltalk running on my BrouHaHa VM. All the Smalltalk VMs I've worked on have supported immediate selectors, and a good thing too :)
best Eliot
|
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by Andreas.Raab
On 4 December 2010 02:21, Andreas Raab <[hidden email]> wrote: > > On 12/3/2010 4:18 PM, Eliot Miranda wrote: >> >> BTW, Mariano's use of SmallIntegers rather than wrappers is an attempt >> to get a very compact image and seems a reasonable experiment. Using >> wrappers would defeat his use-as-little-memory-as-possible purpose. > > Not at all. The design of objects-as-methods was deliberately done such that > one could use a single wrapper for multiple methods. The only thing you need > is to map from selector to method being executed. For example: > > MethodSwapper>>run: aSelector with: argsArray in: aReceiver > "Execute the given method" > method:= methodMap at: aSelector. > ^aReceiver withArgs: argsArray executeMethod: method > > Then you install a single MethodSwapper instance like here: > > swapper := MethodSwapper new. > Morph selectorsAndMethodsDo:[:sel :meth| swapper methodMap at: sel put: > meth]. > Morph selectorsDo:[:sel| Morph addSelector: sel withMethod: swapper]. > > At this point the only overhead you have is what's in the (single) > MethodSwapper instance and even with a most naive implementation (looking up > the index by selector) you'd have 8 bytes per method in your image which I > suspect is well below 5% of the image size. > > And then of course you can start *really* looking at getting the size down. > Did you know that Dan once did a very neat hack where he replaced all > symbols with integers? Works great and if you do that you not only reclaim > the space for the symbols but also don't need a lookup at all :-) > don't need a lookup? how? you still have a method dictionaries, and keys there are not ordered nor sequential ( yes, you can make selectors in a single class to be sequential , like 1 , 2 ,3 ,4 , then you don't need to do a lookup for a single given class, but for the rest of classes which obviously contain different set of selectors, you won't be able to do that). > Cheers, > - Andreas > -- Best regards, Igor Stasenko AKA sig. |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by Andreas.Raab
On Dec 4, 2010, at 2:21 AM, Andreas Raab wrote: > On 12/3/2010 4:18 PM, Eliot Miranda wrote: >> BTW, Mariano's use of SmallIntegers rather than wrappers is an attempt >> to get a very compact image and seems a reasonable experiment. Using >> wrappers would defeat his use-as-little-memory-as-possible purpose. > > Not at all. The design of objects-as-methods was deliberately done such that one could use a single wrapper for multiple methods. The only thing you need is to map from selector to method being executed. For example: > > MethodSwapper>>run: aSelector with: argsArray in: aReceiver > "Execute the given method" > method:= methodMap at: aSelector. > ^aReceiver withArgs: argsArray executeMethod: method > > Then you install a single MethodSwapper instance like here: > > swapper := MethodSwapper new. > Morph selectorsAndMethodsDo:[:sel :meth| swapper methodMap at: sel put: meth]. > Morph selectorsDo:[:sel| Morph addSelector: sel withMethod: swapper]. > But I'm not wrong your solution still requires to have one instance var to keep the id of the compiled methods saved on disc (so you cannot avoid to have the methodMap) with integer the integer itself encodes this information. No need for a map no need for an object. > At this point the only overhead you have is what's in the (single) MethodSwapper instance and even with a most naive implementation (looking up the index by selector) you'd have 8 bytes per method in your image which I suspect is well below 5% of the image size. > > And then of course you can start *really* looking at getting the size down. Did you know that Dan once did a very neat hack where he replaced all symbols with integers? Works great and if you do that you not only reclaim the space for the symbols but also don't need a lookup at all :-) > > Cheers, > - Andreas |
Re: SmallInetger as methods was not ready yet....now maybe?
|
|
In reply to this post by Igor Stasenko
On 12/3/2010 9:12 PM, Igor Stasenko wrote: > don't need a lookup? how? > you still have a method dictionaries, and keys there are not ordered > nor sequential ( yes, you can make selectors in a single class > to be sequential , like 1 , 2 ,3 ,4 , then you don't need to do a > lookup for a single given class, but for the rest of classes which > obviously contain different set of selectors, you won't be able to do > that). Sorry, "no lookup" referred to the initial comment of wanting somehow to map selectors to some number (which I presumed was an index into some database to load the unloaded compiled method). Of course, method lookup is still required. Cheers, - Andreas |
«
Return to Squeak VM
|
1 view|%1 views
| Free forum by Nabble | Edit this page |