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

BUG: cffi type parsers for g-object, g-boxed-foreign, etc... create new types for every binding, every parameter #69

Open
stacksmith opened this issue Jul 18, 2019 · 7 comments

Comments

@stacksmith
Copy link

stacksmith commented Jul 18, 2019

(eq (cffi::parse-type '(g-boxed-foreign gtk-text-iter))
    (cffi::parse-type '(g-boxed-foreign gtk-text-iter)))
NIL

This is not what I expect from a type parser! Compare this behaviour with:

(defcstruct foo (aaa :int)(bbb :int))
(eq (cffi::parse-type '(:struct foo))
    (cffi::parse-type '(:struct foo)))
T

This implies that every mention of a cl-cffi-gtk-managed type in defcfun bindings likely creates a new type inside CFFI machinery. This means thousands of unnecessary types are generated, and type equality is not consistent.
@Ferada - have you come across this?

Edit: implemented in gobject/gobject.base.lisp DEFINE-PARSE-METHOD...
cffi parses types to convert parameters to and from foreign...
I suppose this can go unnoticed since all these types do the same thing... I would have missed it myself - but my CFFI introspection tools use the actual cffi types to reason about parameter types...

@Ferada
Copy link

Ferada commented Jul 18, 2019

I don't think CFFI gives any guarantees about it, at least searching for
"equal" doesn't give me anything from the manual. In fact even the
default :simple-parser argument creates a method that does not create
eq results:

(cffi:define-foreign-type foo () () (:actual-type :int) (:simple-parser foo))
=> expands to
...
  (DEFINE-PARSE-METHOD FOO (&REST CFFI::ARGS)
    (APPLY #'MAKE-INSTANCE 'FOO CFFI::ARGS))
...

You're right though for defctype it's indeed returning the same object
via the closure created by notice-foreign-type.

That said, it sounds to me like it would be okay to cache them, just
where (maybe/not CFFI itself?) is another good question.

Edit: Oh you wrote CFFI introspection tools - if you want that guarantee
you might have to make a PR for CFFI itself I'd say.

@stacksmith
Copy link
Author

stacksmith commented Jul 18, 2019

Hmm. It may be a cffi issue, but it makes no sense at all. If a type parser creates a new type every time it encounters a parameter of say (g-object gtk-text-buffer) type, there are thousands of #<GOBJECT::FOREIGN-G-OBJECT-TYPE...> objects created here. They are then referenced inside the defcfun code and used for conversion and typechecking... This is madness.
I think I must be missing something. A type is a type, an instance is an instance, and parsing a type like (g-object gtk-text-buffer) should return its foreign type, which should be unique for all objects of that type. That's what a type is, after all.
DEFINE-FOREIGN-TYPE should be called once to create a foreign type, and the same foreign type may now be used for all instance objects of this type. I should then be able, given some instance, to get its type, and for god's sakes, it should be the same f***ing type I used when I created it. Regardless of how such a type is specified - as an actual type object or a designator like (gobject gtk-text-buffer). Otherwise why have types at all?
I have to go bang my head against a wall now.

@stacksmith
Copy link
Author

stacksmith commented Jul 18, 2019

I will defend my idea of type equality, and conjecture that this is a cl-cffi-gtk bug.

I think CFFI idea of foreign types represented by a compound designator such as (:STRUCT FOO) -- as well as type equality -- is subtle (and not specified following the CL tradition of letting the user define equality for complex objects)...

There are many different c structures possible, and each one is indeed a different type! However, every instance of (:STRUCT FOO) has the same type as any other instance of (:STRUCT FOO). You can check for yourself. The registered type parser sees (:STRUCT FOO) and correctly returns its type object.

As @Ferada points out, simple parsers just instantiate new types. This makes sense for 'metatypes' like cstruct - every time you define a new cstruct it is in fact a new type, since we do this to define a new type with different slots. This parser is not suitable for compound type descriptors, which is probably why it's called simple.

This makes sense for gobject types, since each gobject type has different slots. This however does not make sense for gobject instances. It makes even less sense to create a new type every time a type is mentioned or parsed. A type parser sees `(G-OBJECT GTK-TEXT-BUFFER), and creates a new type every time. THIS IS COMPLETELY WRONG. This should only happen when a new kind of g-object is being defined.

As I mentioned, this is a subtle bug that eats your resources. The behaviour of all these thousands of GTK-TEXT-BUFFER types is identical, and no one uses CFFI types for typechecking based on identity (except me!). So unless you open the hood and climb inside wearing insane hacker glasses you would never see it.

So yes, I think it is a cl-cffi-gtk bug that should be fixed.

@stacksmith
Copy link
Author

stacksmith commented Jul 18, 2019

This is a confusion between creating an instance of a type and an instance of an object of that type. And this is especially a problem when you don't want to create any instances at all, but pass an existing foreign object as a pointer to a C function that expects a pointer. You certainly don't need a different CFFI type for every such a pointer parameter.

@stacksmith
Copy link
Author

stacksmith commented Jul 19, 2019

Also: are any of these redundant types ever deleted? There is also a slew of activity involving creating types in gobject.boxed-lisp.lisp, for instance, that is simply impossible to follow. It looks like there is a whole other layer of translation involving creating new types... And for slotted objects, this is done for every slot?
EDIT: No, of course they are never deleted. Every binding has a compiled reference to a few of these! See for yourself: copy one of cl-cffi-gtk binding definitions to REPL and compile there, then describe and disassemble. You will see that it creates and compiles a new instance of every g-object-derived parameter, and calls translate-to-foreign generic using it. Also there is an unwind-protect around each such parameter (personally I would make a single one do), and another method dispatch to free each one. And that compiles to about a kilobyte of code, not counting all the temporary type instances, for every binding.
All that to pass a couple of pointers we already have, to existing foreign objects.

GTB> (defcfun ("gtk_text_buffer_insert" x) :void
  (buffer (g-object gtk-text-buffer))
  (iter (g-boxed-foreign gtk-text-iter))
  (text (:string :free-to-foreign t))
  (len :int))
WARNING: redefining GTB::X in DEFUN
X
GTB> (describe 'x)
GTB::X
  [symbol]

X names a compiled function:
  Lambda-list: (BUFFER ITER TEXT LEN)
  Derived type: (FUNCTION (T T T T) (VALUES &OPTIONAL))
  Source form:
    (LAMBDA (BUFFER ITER TEXT LEN)
      (BLOCK X
        (MULTIPLE-VALUE-BIND (#1=#:G720 #2=#:PARAM726)
            (TRANSLATE-TO-FOREIGN BUFFER
                                  #3=#<GOBJECT::FOREIGN-G-OBJECT-TYPE {10058C73D3}>)
          (UNWIND-PROTECT
              (PROGN
               (MULTIPLE-VALUE-BIND (#4=#:G721 #5=#:PARAM725)
                   (TRANSLATE-TO-FOREIGN ITER
                                         #6=#<GOBJECT::BOXED-OPAQUE-FOREIGN-TYPE {10058C7143}>)
                 (UNWIND-PROTECT
                     (PROGN
                      (MULTIPLE-VALUE-BIND (#7=#:G722 #8=#:PARAM724)
                          (TRANSLATE-TO-FOREIGN TEXT
                                                #9=#<CFFI::FOREIGN-STRING-TYPE :UTF-8>)
                        (UNWIND-PROTECT
                            (PROGN
                             (LET ((#10=#:G723 LEN))
                               (CFFI-SYS:%FOREIGN-FUNCALL
                                "gtk_text_buffer_insert"
                                (:POINTER #1# :POINTER #4# :POINTER #7#
                                 :INT #10# :VOID)
                                :CONVENTION :CDECL :LIBRARY :DEFAULT)))
                          (FREE-TRANSLATED-OBJECT #7# #9# #8#))))
                   (FREE-TRANSLATED-OBJECT #4# #6# #5#))))
            (FREE-TRANSLATED-OBJECT #1# #3# #2#)))))

@stacksmith stacksmith changed the title BUG: cffi type parsers for g-object, g-boxed-foreign, etc... create new types every time! BUG: cffi type parsers for g-object, g-boxed-foreign, etc... create new types for every binding, every parameter Jul 19, 2019
@stacksmith
Copy link
Author

stacksmith commented Jul 19, 2019

And CLOS dispatches translate-to-foreign and free-translated-object using these correctly, since each of these foreign type instances has the same CL type.
So each time translate-to-foreign/free-translated-object pair is compiled on a gobject, it uses a different foreign type.

@Ferada
Copy link

Ferada commented Jul 20, 2019

Just out of curiosity I created a patch here that caches these before giving them to CFFI.

I can't measure any noticeable difference (neither in memory usage, nor in speed of loading), maybe you can.

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

No branches or pull requests

2 participants