shoes / shoes3

a tiny graphical app kit for ruby
http://walkabout.mvmanila.com
Other
180 stars 19 forks source link

probable deprecation, soon (might be already done) of Data_Wrap_Struct/Data_Get_Struct #270

Open passenger94 opened 8 years ago

passenger94 commented 8 years ago

https://github.com/ruby/ruby/blob/6a69ab937c7e6584d311cd444615cc1cd5b5499b/doc/extension.rdoc#encapsulate-c-data-into-a-ruby-object

– The old (non-Typed) Data_XXX macro family has been deprecated. In the future version of Ruby, it is possible old macros will not work. ++

This will impact every corner of C Shoes ... We have to switch to the new API : TypedData_Wrap_Struct / TypedData_Get_Struct

Lots of reviewing but hopefully nothing harmful ...

Not related i think ... i can compile Shoes against ruby 2.3.1 but fails to launch somewhere in rb_lod_path ... (just me ?)

ccoupe commented 8 years ago

I haven't tried 2.3. There is/was some hardcoded nonsense in cache.rb/shoes.rb that might need adjusting. and some rakefiles.

I'm not surprised that the Ruby Gods are moving the goal posts until no-one can embed Ruby. I should probably rejoin the Ruby ML.

passenger94 commented 8 years ago

well, the "new APi" is there since 1.9 apparently, so Gods have been patient this time :-D

passenger94 commented 8 years ago

made some tests, i think i can handle it, it works on video widget, still some tiny Macro to think about,to be as generic as possible ... Your beloved Dragons are lying in every existing Macro which might have a "hidden" Data_Get_Struct, Gigantic boring copy/paste orgy in sight ...

Basically : define a dsize function (fetch the size of the wrapped Struct) and a rb_data_type_t struct for every underlying C struct of a ruby object in alloc function change accordingly from Data_Wrap_Struct to TypedData_Wrap_Struct everywhere change Data_Get_Struct to TypedData_Get_Struct for video widget :

static size_t video_dsize(const void *vid)
{
    return sizeof(shoes_video);
}

const rb_data_type_t shoes_video_type = {
    "video_type",
    {
      (void (*)(void *))shoes_video_mark, 
      (void (*)(void *))shoes_video_free, 
      video_dsize,
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY, // <--- this one needs your thoughts
};

VALUE shoes_video_alloc(VALUE klass) {
  shoes_video *video = SHOE_ALLOC(shoes_video);
  SHOE_MEMZERO(video, shoes_video, 1);

//  VALUE obj = Data_Wrap_Struct(klass, shoes_video_mark, shoes_video_free, video);
  VALUE obj = TypedData_Wrap_Struct(klass, &shoes_video_type, video);
  video->attr = Qnil;
  video->parent = Qnil;
  video->realized = 0;
  return obj;
}

VALUE
shoes_video_get_realized(VALUE self) {
  shoes_video *self_t;
//  Data_Get_Struct(self, shoes_video, self_t);
  TypedData_Get_Struct(self, shoes_video, &shoes_video_type, self_t);

  return self_t->realized ? Qtrue : Qfalse;
}

PS : it's deprecated since 2.2 !

ccoupe commented 8 years ago

Lets think very hard before doing anything that would cause such a massive git merge (doubling the cut/paste work for all the conflicts). I believe C can spit out the size of a struct without calling a function that call sizeof. That might simplify some things.

passenger94 commented 8 years ago

sure, working on a separate branch, merging when really confident ! ha yes, most people are just issuing a 0 in place of the dsize

ccoupe commented 8 years ago

More issues that just a different branch. it's a freeze on any branch that has a conflict. If the rewrite is to the old Ruby style instead of the new gc write barrier stuff, can we use both old and new style, widget by widget, function by function?

passenger94 commented 8 years ago

the funny thing is that you can access the underlying struct directly now, bypassing the TypedData_Get_Struct, which as i understand it is supposed to be safer : there is a RTYPEDDATA_DATA(rubyobject) Macro for that .... returning a void pointer to the struct

passenger94 commented 8 years ago

oh, no need for write barrier, only if you want the new gc generational feature

passenger94 commented 8 years ago

yes we can mix both styles, that was my plan, going slowly widget by widget, fix problems, commit, move on

There is some real fun ! for example in shoes_canvas_draw function you have to guard (check if ele is a cCanvas) before calling TypedData_Get_Struct(ele, shoes_canvas, c1); otherwise it won't work, in the old api, the check was done by the Macro .... >_> (took me a while to figure out this one)

ccoupe commented 8 years ago

If we can mix them then it does make things easier. Just co-ordinate which widgets are being upgraded so we don't collide. BTW sizeof(type) is a C operator so this should work

const rb_data_type_t shoes_video_type = {
    "video_type",
    {
      (void (*)(void *))shoes_video_mark, 
      (void (*)(void *))shoes_video_free, 
      sizeof(shoes_video)
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY, // <--- this one needs your thoughts
}

My understanding of the doc is that RUBY_TYPED_FREE_IMMEDIATELY is the backwards compatible method, the one we want.

ccoupe commented 8 years ago

I'm going to merge the graph branch into master even though the plot widget is only 80% complete (but semi-usable) and it has the #240 fixes; You can then merge master into your branch and later collisions will be minimized, I hope. I added one more method to plot and 'll try the new api on it and get some experience.

passenger94 commented 8 years ago

yes, but someone wants a cast ! ... this works on video widget

const rb_data_type_t shoes_video_type = {
    "video_type",
    {(void (*)(void *))shoes_video_mark, 
      (void (*)(void *))shoes_video_free, 
      (size_t (*)(const void *))sizeof(shoes_video),
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY,
};

let's go with RUBY_TYPED_FREE_IMMEDIATELY (my understanding is this tells ruby to free the data_type_t struct as soon as the shoes_video struct is free-ed - ie not waiting a GC collection - so it looks adequate.

Maybe there is no emergency to integrate plot into master (in case you want to quietly polish your creation) : we are going to learn a lot about the real implications of this change, so later it could ease the integration, knowing where to look for problems, just a thought ...

passenger94 commented 8 years ago

we need to put redirection flow in some methods, like shoes_basic_remove, the time of transition, there is a Macro which checks if an object is Typed or not : RTYPEDDATA_P(rubyobject)

SETUP_BASiC Macro needs a revamp, because we can't access shoes_basic struct like before this looks working good : shoes_basic* basic = (shoes_basic*)RTYPEDDATA_DATA(rubyobject) and we can access parent, attr members like in old api

ccoupe commented 8 years ago
#define VIDEO_STRUCT_SZ (size_t (*)(const void *))sizeof(shoes_video)
const rb_data_type_t shoes_video_type = {
    "video_type",
    {(void (*)(void *))shoes_video_mark, 
      (void (*)(void *))shoes_video_free, 
     VIDEO_STRUCT_SZ ,
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY,
};

Only a thought. Too late on the merge to master. I'm going to let you figure out the new api on your widget of choice while put the finishing touch on plot with the old api.

passenger94 commented 8 years ago
#define TYPED_STRUCT_SZ(base)  (size_t (*)(const void *))sizeof(base)

not tested yet

ccoupe commented 8 years ago

Very good.

passenger94 commented 8 years ago

created a typeddata branch on my repository, it almost works on video widget, except some glitches while drawing, noticeable on the volume knob (doesn't refresh correctly) and video tests are failing erratically, i suspect some Macro somewhere which doesn’t appreciate avant-garde experiments...

passenger94 commented 8 years ago

hello, If you have some time, could you check, please, names, macros and the general idea, before i go too far, shoes_video and shoes_app are done, and of course as usual there is cocoa...(changes are there with commented ancient code). no hurry

ccoupe commented 8 years ago

If you create a remote branch and push to it, then I can pull your changes down.

passenger94 commented 8 years ago

yep, on my repository, updated to your latest commits now : branch typeddata (did you mean creating one on your repository ?)

ccoupe commented 8 years ago

Yeah, I was expecting the new branch on my repo. I'm pretty sure I do not want to do a simple clone your repo - that would be confusing for me.

passenger94 commented 7 years ago

Hello @ccoupe, did you got a chance to look at the proposed changes ? i might have some times to continue ...

ccoupe commented 7 years ago

Haven't had the chance. I keep finding errors with my chart code ;-) And moving goal posts.

ccoupe commented 7 years ago

Video tests, for me, report the same volume control err they did on the master branch. I don't see any new problems with video for the typeddate branch. Svg might be a place to try your new macros on. Do you know if ffi is using new code or old code?

passenger94 commented 7 years ago

Ok i'll tackle that (svg), i just wanted to know if the new Macros are acceptable like they are now (names, functionality, ...), as you obviously know, later it would mean redoing the whole thing ... again...
app was fine too, apparently .... sounds like an approval :-)

Video tests, for me, report the same volume control err

? what is the error.

passenger94 commented 7 years ago

forgot to mention, Macros of interest are here : https://github.com/Shoes3/shoes3/blob/typeddata/shoes/ruby.h#L142

here is a summarize of what is involved :

Calling TypedDATA_type_new(...) for every wrappd struct, which creates the new type struct; for example with shoes_app : TypedDATA_type_new(shoes_app) creates

const rb_data_type_t shoes_app_type = {
    "shoes_app_type",
    {
      (void (*)(void *))shoes_app_mark,
      (void (*)(void *))shoes_app_free,
      (size_t (*)(const void *))sizeof(shoes_app),
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY,
}

then replacing old GET_STRUCTby GET_TypedSTRUCT ........// unwraps implicit self also old Data_Get_Struct by GET_TypedSTRUCT2 ............// unwraps explicit ruby object and old Data_Wrap_Struct by TypedData_Wrap_Struct

plus the new way to access beginning part of a struct via struct shoes_basic (talked about in earlier comment, above)

tedious but easy, the hard part is finding hidden side effects (mostly in Macros) ... (and no, i won't complain about missing tests because santa is too far away)

Edit instead of

  VALUE obj = shoes_svg_alloc(cSvg);
  shoes_svg *self_t;
  Data_Get_Struct(obj, shoes_svg, self_t);

now i propose (less verbose and coherent with GET_STRUCT / GET_TypedSTRUCT) self_t is declared/assigned inside the Macro

  VALUE obj = shoes_svg_alloc(cSvg);
  GET_TypedSTRUCT2(obj, shoes_svg, self_t);

But while we are at it, personally, i would really like shoes_svg *self_t = GET_TypedSTRUCT2(obj, shoes_svg); (that way self_t is not hidden inside the Macro, idem for GET_TypedSTRUCT )

Also instead of things like

GET_STRUCT(svg, svg);

i propose

GET_TypedSTRUCT(shoes_svg, svg);

type the whole type ! not only part of it, i find it often confusing

ccoupe commented 7 years ago

I was going to complain about the mixed case Macro names but I see that Ruby has decided it's good for them. We can adapt.

I too find the GET_STRUCT(svg, svg) confusing and prefer both of your suggestions. GET_TypedSTRUCT(shoes_svg, svg); and your shoes_svg *self_t = GET_TypedSTRUCT2(obj, shoes_svg);`

passenger94 commented 7 years ago

recap of options for unwrapping Macros : (of course, shoes_svg and svg are just examples, all options compiles and work for me)

A - as of now, 2 Macros (for implicit self and whatever ruby object) hidden declaration/assignment of variable svg inside Macro, less verbose GET_TypedSTRUCT(shoes_svg, svg); GET_TypedSTRUCT2(rubyObj, shoes_svg, svg); ... do stuff with svg variable

B - 2 Macros explicit declaration of variable svg shoes_svg *svg = GET_TypedSTRUCT(shoes_svg); shoes_svg *svg = GET_TypedSTRUCT2(rubyObj, shoes_svg); ... do stuff with svg variable

C - 1 Macro, tiny bit more verbose in the self case (seen very often in code) shoes_svg *svg = GET_TypedSTRUCT(self, shoes_svg) ) shoes_svg *svg = GET_TypedSTRUCT(rubyObj, shoes_svg); ... do stuff with svg variable

either A or C for me (looking at this, maybe A is better, finally)

Note, there is some few cases where use of those Macros is not appropriate, for example if we must declare the variable beforehand, there we use the original ruby Macro : TypedData_Get_Struct (instead of old Data_Get_Struct) !! easy to forget, when looking up for changes to do in code ...

about Macros in all Caps convention, as visual feedback, those would become : GET_TYPEDSTRUCT TYPEDDATA_TYPE_NEW

again no obvious choice for me, all Caps has the advantage to disambiguate ruby's Macro from Shoes's one, though

so, it seems i have some proclivity (didn't knew one could have proclivity, hope it's not contagious !) for : TYPEDDATA_TYPE_NEW(shoes_svg) GET_TYPEDSTRUCT(shoes_svg, svg); GET_TYPEDSTRUCT2(rubyObj, shoes_svg, svg);

waiting for your feedback :-)

passenger94 commented 7 years ago

Svg is working now, but i discovered that something with the new API interacts badly with the forced garbage collection inside shoes_svg_set_handle, commenting out the call to rb_gc() "fix" the problem but Shoes leaks or disgustingly bleeds i should say ...... (testing with good-cardflip.rb)

I hope it's not related to the new generational gc ...... if so, that's good news for our go back to school plan ....

ccoupe commented 7 years ago

It could be a Shoes problem. In theory, that rb_gc call shouldn't be there. But, it is there for a reason.

Perhaps the svghandle code is holding on to something that the new gc sees as being in use. Animate may also be part of the problem. It leaks little bits. If you left the old wavy line Shoes splash screen running overnight - no memory in the morning.

ccoupe commented 7 years ago

I'm OK with all Caps for Shoes macros and mixed case for Ruby, Are your new macros Shoes or Ruby? I think they are more Ruby than Shoes. I think I prefer mixed case. Get_Data_Struct is already in heavy use in Shoes. We all know it's a Macro and not a function. We also know, the ruby core would like to get rid of RSTRING_PTR and other's like it. They just happen to have a barn load of old code with those old macros in their code base so it hasn't happened yet. But it will.

passenger94 commented 7 years ago

TypedData_Type_New(shoes_svg); Get_TypedStruct(shoes_svg, svg); Get_TypedStruct2(rubyObj, shoes_svg, svg);

Shall we summon some casual plain sacrificial ceremony, with just few innocent offerings and call those Macros official templates ?

ccoupe commented 7 years ago

I sacrifice some beers. I'm happy with the names.

passenger94 commented 7 years ago

made a new PR (synched with master), there's conflicts with your typeddata branch, i don't know why, mine is ok with your master synchronized, i can create another clean branch in your repo if you want

I'm looking into that cardflip problem, it's not directly related to the new API (it happens too in old API, rarely, but happens , so it went unnoticed...) i fear race/threading problems ... new API probably shed some crude light on it

ccoupe commented 7 years ago

Go ahead and create a new branch if like.

passenger94 commented 7 years ago

I fixed the svg handle problem, it was the shoes_svghandle->subid string which was garbage collected, it was assigned the pointer to the ruby string directly, so ruby collected it at some point and no more pointed data ... my bad, i forgot to copy/duplicate the char* self_t->subid = strdup(RSTRING_PTR(subidObj)); fixed it (PR on master to come, for just that)

now, it is still leaking, i can't manage to see where, it's clearly the svghandles that are not properly deleted/freed but ... it stabilize at around 400 Mo !! on my system when the full Deck is given a handle per card, but if you load a new Deck you're set up for another 400 Mo ! etc... Manually calling the GC (in ruby) just after a new deck file, helps somewhat, it stays at 400 Mo, but i was expecting memory load to go back to around initial 20/25 Mo, nope ! (Note that calling GC only after a second deck complete handles load, one can't go below 800Mo, etc...)

PS May i delete old typeddata branch ?

ccoupe commented 7 years ago

Delete the branch when you like. If you look at librsvg C code you'll see that if you have 52 handles active (subids) then you will have 52 full copies of the whole file (each with 52 xml drawing instructions). As I remember, subid/group copies the whole thing. There is no great speed benefit to using handles either, in Shoes. So, memory will stabilize at some high point.

It might be that librsvg does reference counting and Shoes isn't unref-ing properly - which would leak a lot of memory. I did not see that in the documentation but you know how doc works.

passenger94 commented 7 years ago

yes, i got this about the way handles work, i'm not that much surprised by the size of the memory load once all handles are created, it's the fact that memeory don't get released when deleting the handles. Yes it's reference counted, but in code they do get unreferenced in _shoes_svghandlefree i didn't find a way to check the ref count, though, so maybe the ref count is greater than one, so never reach 0 and handles don't get released, other than that, i'm out of ideas ....

ccoupe commented 7 years ago

Assuming they use gobject (pretty safe assumption), this article says g_object_run_dispose() can really unref.

passenger94 commented 7 years ago

...atm nothing better, no matter.... except rsvg handles DO get freed (just call unref twice, 2nd times it complains about missing object, always), scratching head ... there must be something else ...

But at least we can move on with new API ! :-)

Also, not related to the leaks i think, there's more to the game :

It is recommended that klass derives from a special class called Data (rb_cData) but not from Object or other ordinal classes. If it doesn't, you have to call rb_undef_alloc_func(klass).

my understanding is that rb_undef_alloc_func(klass) disallows the use of new keyword in ruby side rb_cData is defined here : https://github.com/ruby/ruby/blob/3827c23af571895a76153c006a07136704c3d33e/object.c#L3590 just a subclass of Object with rb_undef_alloc_func(klass)

it replaces rb_define_alloc_func https://github.com/ruby/ruby/blob/a23878c3f5f78522d1c977ff59df936a16abcaa6/vm_method.c#L680

indeed we never call new on our extension Ruby classes

example in ruby : https://github.com/ruby/ruby/blob/468301b98487d3b2b0d9e4a60c912803f4ba39f0/string.c#L9945

and another reading of interest : we don't need write barriers (complicated) but ... https://github.com/ruby/ruby/blob/43112104dffdcee69628d60dd78d761e8ea650ae/doc/extension.rdoc#write-barriers

ccoupe commented 7 years ago

We don't call new() in ruby but every Shoes 'widget' does have an alloc and it is called (automagically) and there is a lot of setup inside them. We also need inheritance from the Shoes classes. There might be some internal Shoes objects that could use rc_cData but most of Shoes is much farther up the tree than Object.

I must be missing what your point is.

passenger94 commented 7 years ago

well, alloc is not called automagically, but explicitly in all shoes_whatever_new() method ... and this is all happening in C side, so disabling new in Ruby has no impact. The poor hardscrabble wormling Me fears that above rb_cObject there is only mystical entities that, we, human being, are not allowed to talk about ... As it is not directly related to the new API, you can try it now, it just works ! for example :

  // cTextBlock = rb_define_class_under(cTypes, "TextBlock", rb_cObject);
  // rb_define_alloc_func(cTextBlock, shoes_textblock_alloc);
  cTextBlock = rb_define_class_under(cTypes, "TextBlock", rb_cData);
  rb_undef_method(CLASS_OF(cTextBlock), "new")

is the same as :

  cTextBlock = rb_define_class_under(cTypes, "TextBlock", rb_cObject);
  rb_undef_alloc_func(cTextBlock);
  rb_undef_method(CLASS_OF(cTextBlock), "new");

and later (inheritance)

  cPara = rb_define_class_under(cTypes, "Para", cTextBlock);
  rb_undef_method(CLASS_OF(cPara), "new");

Before, in Ruby, you could write p = Shoes::Para.new() but p is just an allocated object but not initialized (methods of the object are there but useless) After, if you try p = Shoes::Para.new() you'll have : undefined method "new" for Shoes::Types::Para:Class

Indeed it is not critical, to say the least, just cleaner IMHO, but while we are at it why not make it so that we don't unnecessarily fight against mainstream ruby protocols.

ccoupe commented 7 years ago

Cleaner (in some ways, IMO) would be for Shoes::para.new("my para", {options} to create a para in the calling slot -actually work. However, I suspect that is a deep and twisty hole with many dead ends. I'm reluctant to deal with that part of Shoes unless we absolutely have to.

There is a related issue: svghandle and the new chart_series really should not require an object creation method on 'app' like app.svghandle and app.chart_series. It's probably too late for svghandle but I'm thinking that chart_series should be just another Ruby Class not in the Shoes name space. Only available in Shoes' Ruby. Time to experiment.

passenger94 commented 7 years ago

svghandle and the new chart_series really should not require an object creation method on 'app'

you won't be surprised if i agree ;-)

for the new method undef, it's exactly what you want, forget the method (useless) and don't make it available to Ruby, why should we want Shoes::Para.new(...) if we have splendid para(...)

ccoupe commented 7 years ago

What I forget as often as anybody else is that "Shoes is not Ruby - it only looks like Ruby!". So far, I have not found the magic to match. I want to call ChartSeries.new opton1: value1, option2: value2 instead of app.chart_series opton1: value1, option2: value2 Not as simple as I think.

passenger94 commented 7 years ago

Maybe i don't understand what you want to do but i don't get why you want ChartSeries.new() instead of typical shoes chart_series(), if it's to get rid of the app, there's redirection methods from app to canvas, also for non graphical object for example timer : https://github.com/Shoes3/shoes3/blob/5e2041993069fa4e42d20037e311875d5e61b26f/shoes/ruby.h#L254 just don't write a draw method if not necessary. ?? If a strait Ruby Class is the way to go, why not do it in Ruby if you don't have speed requirements. It's just an entity to encapsulate Data, no ? ( maybe you want to keep everything plot related in the same place ?)

ccoupe commented 7 years ago

OK, I'm back on this issue. Let's see if can summarize the work to be done.

Is that correct, @passenger94

passenger94 commented 7 years ago

looks correct, i need to re immerse to be sure though ...

Wraping : // creates struct shoes_svg_type TypedData_Type_New(shoes_svg);

and in shoes_svg_alloc : obj = TypedData_Wrap_Struct(klass, &shoes_svg_type, svg);

Unwrapping: (Both Macros declares the pointer to the C struct internally)

(there's also Get_TypedStruct3 which returns the struct : shoes_svg *cstruct = Get_TypedStruct3(rubyObject, shoes_svg) )

to be done on every rb_cObject , so yes for svg there is two of them except the boring aspect of it, it's quite straightforward, problem is code hidden away in Macros

as for rb_undef_method i've made some tests and didn't hit any problem ... we don't use new and even though it is possible at the moment (doesn't crash), objects are only allocated but not initialized, they have no consistency (sort of zombies objects), so useless

ccoupe commented 5 years ago

Things have gotten 'complicated' since @passenger94 wrote the macro replacements. That was before the new file scheme @BackOrder created. Now it is glaringly obvious just how 'inter-twingled' they are. Just about every Macro needs a duplicate and some #ifdef's to select between them. Picking svg or plot is the easiest since they have very little 'twingle' Every file/shoes object will have to be touched at least twice. - Once to do the above, Getting the object to work, and then when all objects are converted and working, removing the #ifdef's. Tedious and it is not as simple as cut and paste or mass batch edits.

ccoupe commented 5 years ago

I had to resort to some trickery to get svg (or any other widget) to draw without hanging/crashing. In file canvas.c: 572 - function shoes_canvas_draw(), it blindly unwraps widgets or any type into a shoes_canvas. That fails big time on objects created with the new macros. This is not the only place in Shoes - I know I did something like that somewhere too. So, for doc purposes, here's a trick to get past the type checking.

            VALUE ele = rb_ary_entry(self_t->contents, i);
            if (RTYPEDDATA_P(ele)) {
              /* This a nasty trick. The original blindly assumes that
               * Data_Get_Struct can unwrap anything into a shoes_canvas.
               * shoes_element would be a better choice but they both fail
               * for TypedData objects
              */
              c1 = (shoes_canvas *)RTYPEDDATA_DATA(ele);
            } else {
              Data_Get_Struct(ele, shoes_canvas, c1);
            }