On Sat, Aug 27, 2016 at 09:59:51AM +0900, Carsten Haitzler wrote:
> On Fri, 26 Aug 2016 17:34:25 +0200 marcel-hollerb...@t-online.de said:
>
> > Hello,
> >
> > On Mon, Aug 22, 2016 at 08:04:09PM -0700, Carsten Haitzler wrote:
> > > raster pushed a commit to branch master.
> > >
> > > http://git.enlightenment.org/core/efl.git/commit/?id=561f8eaa8faebe32b9a03cf9674c147cf0d6d9ab
> > >
> > > commit 561f8eaa8faebe32b9a03cf9674c147cf0d6d9ab
> > > Author: Carsten Haitzler (Rasterman) <ras...@rasterman.com>
> > > Date: Tue Aug 23 11:59:37 2016 +0900
> > >
> > > efreet - save about 240-300k or so of memory used by efreet mime
> > >
> > > so efreet mime was loading a bunch of mime type info files, parsing
> > > them on startup and allocating memory to store all this mime info -
> > > globs, mimetype strings and more. all a big waste of memory as its
> > > allocated on the heap per process where its the SAME data files loaded
> > > every time.
> > >
> > > so make an efreet mime cache file and a tool to create it from mime
> > > files. mmap this file with all the hashes/strings in it so all that
> > > data is mmaped once in memory and shared between all processes and it
> > > is only paged in on demand - as actually read/needed so if your
> > > process doesnt need to know about mime stuff.. it wont touch it
> > > anyway.
> > >
> > > this saves about 240-300k or so of memory in my tests. this has not
> > > covered the mime MAGIC files which still consume memory and are on the
> > > heap. this is more complex so it will take more time to come up with a
> > > nice file format for the data that is nicely mmaped etc.
> > >
> > > @optimize
> > > ---
> >
> > [snip]
> >
> > > +
> > > EAPI int
> > > efreet_mime_init(void)
> > > {
> > > @@ -194,14 +386,15 @@ efreet_mime_init(void)
> > > }
> > >
> > > efreet_mime_endianess = efreet_mime_endian_check();
> > > -
> > > - monitors = eina_hash_string_superfast_new(EINA_FREE_CB
> > > (ecore_file_monitor_del)); -
> > > efreet_mime_type_cache_clear();
> > >
> > > + _efreet_mimedb_update();
> > > +
> > > if (!efreet_mime_init_files())
> > > goto unregister_log_domain;
> > >
> > > + _efreet_mime_update_func = _efreet_mimedb_update;
> > > +
> > > return _efreet_mime_init_count;
> > >
> > > unregister_log_domain:
> > > @@ -228,6 +421,9 @@ efreet_mime_shutdown(void)
> > > if (--_efreet_mime_init_count != 0)
> > > return _efreet_mime_init_count;
> > >
> > > + _efreet_mimedb_shutdown();
> > > + _efreet_mime_update_func = NULL;
> > > +
> > > efreet_mime_icons_debug();
> > >
> > > IF_RELEASE(_mime_inode_symlink);
> > > @@ -241,10 +437,7 @@ efreet_mime_shutdown(void)
> > > IF_RELEASE(_mime_application_octet_stream);
> > > IF_RELEASE(_mime_text_plain);
> > >
> > > - IF_FREE_LIST(globs, efreet_mime_glob_free);
> > > IF_FREE_LIST(magics, efreet_mime_magic_free);
> > > - IF_FREE_HASH(monitors);
> > > - IF_FREE_HASH(wild);
> > > IF_FREE_HASH(mime_icons);
> > > eina_log_domain_unregister(_efreet_mime_log_dom);
> > > _efreet_mime_log_dom = -1;
> > > @@ -387,11 +580,10 @@ efreet_mime_magic_type_get(const char *file)
> > > EAPI const char *
> > > efreet_mime_globs_type_get(const char *file)
> > > {
> > > - Eina_List *l;
> > > - Efreet_Mime_Glob *g;
> > > char *sl, *p;
> > > - const char *s;
> > > - char *ext, *mime;
> > > + const char *s, *mime;
> > > + char *ext;
> > > + unsigned int i, n;
> > >
> > > EINA_SAFETY_ON_NULL_RETURN_VAL(file, NULL);
> > >
> > > @@ -406,25 +598,27 @@ efreet_mime_globs_type_get(const char *file)
> > > while (p)
> > > {
> > > p++;
> > > - if (p && (mime = eina_hash_find(wild, p))) return mime;
> > > + if (p && (mime = _efreet_mimedb_extn_find(p))) return mime;
> > > p = strchr(p, '.');
> > > }
> > > }
> > >
> > > - /* Fallback to the other globs if not found */
> > > - EINA_LIST_FOREACH(globs, l, g)
> > > + // Fallback to the other globs if not found
> > > + n = _efreet_mimedb_glob_count();
> > > + for (i = 0; i < n; i++)
> > > {
> > > - if (efreet_mime_glob_match(file, g->glob))
> > > - return g->mime;
> > > + s = _efreet_mimedb_glob_get(i);
> > > + if (efreet_mime_glob_match(file, s))
> > > + return _efreet_mimedb_glob_mime_get(i);
> >
> > The upper line is introducing a problem.
> >
> > Before the return was a stringshare, now it is not anymore.
> > I am not so sure if it is a good idea to fix it with
>
> the api docs do not say it is a stringshare string, thus relying on this is a
> violation of api contract ... for exactly the kind of optimizations i've done
> here. efreet returns lots of strings. all except 2 are NOT defined to be
> stringshare strings. :) this is because freet gets a lot of strings DIRECTLY
> from mmaped cache files for desktop files, icon search databases and so on.
> this is "regular how efreet works" stuff. so it's also not a design pattern in
> efreet. in fact the design pattern has ben the opposite. efreet has been
> returning strings to mmaped files for a huge number of it's apis for a long
> time. but to specs...
Problem is more, look at the two last commits in enlightenment, until
right now people uesd it and it perfectly worked. Also its (in my opinion)
much nicer to do "if (mime1 == mime2) { ..." than doing
"if (!strcmp(mime1 == mime2)" (also in terms of performance).
Also if there is absolutly no assertion that a stringshare is returned,
why are then even static strings stringshared in efreet_mime.c?
>
> from the api specs:
>
> /**
> * @param file The file to check the mime type
> * @return Mime type as a string.
> * @brief Retrieve the mime type of a file using globs
> */
> EAPI const char *efreet_mime_globs_type_get(const char *file);
>
I know, problem is (maybe its just me), i often look at the code before
using the function. Its often that functions are not really good
documented. So looking at the function itself gives you a deeper
understanding of the function, and if you then see that even static
strings are returned as stringshares, you 100% go for "Okay this thing
returns stringshares".
> does not document to say it's a stringshare and does not return
> Eina_Stringshare * which would also say that. :)
>
> > return eina_stringshare_add(_efreet_mimedb_glob_mime_get(i));
>
> hmm. no. this would be a leak. the code before returns a string DIRECTLY from
> an internal hash. if we return a stringshare_add to avoid a leak the caller
> would have to stringshare_del when done. if the caller did not call
> stringshare_del like old code would do the refcount of this string would
> forever increase every use and thus basically be unable to be removed from
> memory if no longer needed/used. even worse it'd continue until the reference
> counter in stringshare loops over to like 1 then someone dos a stringshare_del
> on that string and everyone's pointers to this stringshare are not invalid and
> next access they crash or something bad... because the counter now has no
> meaning as it looped over because someone just kept reffing a string
> blindly. :)
>
> > So i am letting you know this was here :)
>
> i know. but code depending on it being a stringshare is "wrong". :) documented
> nowhere to guarantee this. :)
But worked the last decade like this ... AND efreet_mime.c is pretty
much build to return stringshares everywhere, even static strings.
>
> > Greetings
> > bu5hm4n
> >
> >
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > enlightenment-devel mailing list
> > enlightenment-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> >
>
>
> --
> ------------- Codito, ergo sum - "I code, therefore I am" --------------
> The Rasterman (Carsten Haitzler) ras...@rasterman.com
>
------------------------------------------------------------------------------
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
