Page MenuHomePhabricator

Compile a keymap using a file
AbandonedPublic

Authored by JHyun on Jan 12 2016, 2:11 AM.

Details

Summary

Currently, the enlightenment compile a keymap in every booting.

But if a keymap is not changed, that is a burden for booting time.
To reduce booting time, we can compile a keymap using keymap file.(not rmlvo)
So in this patch, I compile a keymap at only first booting and save that keymap to file.
(File path: /var/lib/xkb/, File naming rule: {rules}-{model}-{layout}-{variant}-{options}.xkb)
After make a keymap file, the enlightenment is just compile keymap using file.
Test Plan

If the enligthenment using new rmlvo(same as first time booting),

a keymap file is generated in /var/lib/xkb/ and
using it next time.

Diff Detail

Repository
rE core/enlightenment
Branch
work
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1052
Build 1117: arc lint + arc unit
JHyun updated this revision to Diff 8114.Jan 12 2016, 2:11 AM
JHyun retitled this revision from to Compile a keymap using a file.
JHyun updated this object.
JHyun edited the test plan for this revision. (Show Details)
JHyun added reviewers: raster, devilhorns, zmike.
JHyun added subscribers: ohduna, input.hacker, cedric.
devilhorns requested changes to this revision.Jan 12 2016, 6:31 AM
devilhorns edited edge metadata.
devilhorns added inline comments.
src/bin/e_comp_wl_input.c
330

if (e_config->xkb.use_cache == EINA_FALSE) return;

698

missing a space here after 'keymap'

This revision now requires changes to proceed.Jan 12 2016, 6:31 AM
JHyun updated this revision to Diff 8116.Jan 12 2016, 8:45 PM
JHyun edited edge metadata.

Change some syntax error and API's arguments

JHyun updated this revision to Diff 8117.Jan 12 2016, 9:32 PM
JHyun edited edge metadata.

Initialize a xkb_rule_names variable

JHyun updated this revision to Diff 8120.Jan 12 2016, 9:57 PM
JHyun edited edge metadata.

Check keymap cache file is exist to prevent unnecessary write.

JHyun added a comment.Mar 23 2016, 7:08 PM

Hello.
What do you think of this commit?
This commit is old, so I think it will occur conflict with newest master branch codes. (I'm not sure..)
If you think this commit is okay, I'll rebase this commit.

zmike added a subscriber: bu5hm4n.Mar 24 2016, 9:42 AM

Adding @bu5hm4n because he's been doing some work related to these areas.

@zmike this makes perfect since, after my patch this gets even easier.

Only the part with e_comp_wl_input_keymap_compile is needed there. Since e_comp_wl_input_keymap_set is called before ecore_drm_init is called. so you dont have to change _e_mod_drm_keymap_set. And the only changes are needed in e_comp_wl_input.c

But I am unsure about a few things with the code:

  1. Why using a stringshare as return type ? It adds some overhead just to create the filename, cannot we use something like tmpstring or a simple macro ?
  2. e_comp_wl_input_keymap_path_get is writing a char, which is all the time NULL it doesnt get a value, so i guess your cache is always empty.
  3. In general, what could lead to different xkb_keymaps when with the same parameters, like some files of the libaray change, should we purge those files at some point?
  4. You have a compile and a cache_create method, cannot we have a method called xxxx_get which will compile and cache if it is not a cache yet, or just read the cache and return the value ?

Otherwise, cool idea :)

Thanks for your opinion.
Although you give to me kindly explanation, I have short eng skill, so I have a difficult to understand your comment.
So please check my opinion is right or not.

Only the part with e_comp_wl_input_keymap_compile is needed there. Since e_comp_wl_input_keymap_set is called before ecore_drm_init is called. so you dont have to change _e_mod_drm_keymap_set. And the only changes are needed in e_comp_wl_input.c

: It means there are no changes required in wl_drm module because e_comp_wl_input_keymap_set is already called before wl_drm module is initialize. But I just know that e_comp_wl_input_keymap_set() is called in e_modapi_init() at wl_drm module. So I needed to compile a keymap before e_comp_wl_input_keymap() because maintain same context and keymap in efl lib.

  • Why using a stringshare as return type ? It adds some overhead just to create the filename, cannot we use something like tmpstring or a simple macro ?

: I just use stringshare type for using efl API. It is not bad using char * instead. I'll fix to not use eins_stringshare type.

  • e_comp_wl_input_keymap_path_get is writing a char, which is all the time NULL it doesnt get a value, so i guess your cache is always empty.

: Do you mean that do not return char * directly? using like this?

char * a = eina_stringshare_printf(~~~~~)
if (a) return a;
  • In general, what could lead to different xkb_keymaps when with the same parameters, like some files of the library change, should we purge those files at some point?

: I know that is known issue... If someone change keymap file (xkb keymap files), he / her must remove cached files.
But I think just who have authority to change xkb keymap files can remove cached files. I just think this feature's purpose is enhance booting speed(after 2nd booting) in merely keymap changed environment. So I just think remove cached files is proposal of user. How about your opinion? Are there way to know that xkb keymap files are changed?

  • You have a compile and a cache_create method, cannot we have a method called xxxx_get which will compile and cache if it is not a cache yet, or just read the cache and return the value ?

: I don't understand what you mean T.T. To get libxkbcommon's keymap, it is very fast way to get keymap using keymap file. So I choose make keymap cache file and read from this. Please more specific comment to me? I'm ready to hear your opinions.

Above all, Thanks to your opinions.
That makes me cheer up.
I will wait you opinions.

bu5hm4n added a comment.EditedMar 28 2016, 10:56 PM

I think to understand best, you should take a look at the current state in master. And try to rebase this patch on the current master.

A few word to the current state:

  • e_xkb is the only place where the keymap is setted,
  • it is called before the compositor (and ecore_drm) is initializised.
  • there is nothing with use_dflt_xkb (e_comp_wl_input_keymap_set(const char *rules, const char *options, const char *model, const char *layout) is the signiture)

( I added inline comments, maybe a sentences on the line of code is a bit easier to understand)

src/bin/e_comp_wl_input.c
405

Lets take a look what happens here.

if _e_comp_wl_input_keymap_update is called:

  • tmp = NULL
  • ... (nothing with tmp happens)
  • _e_comp_wl_input_keymap_cache_create(keymap_path, tmp)

So in the end you are saving NULL all the time.

650

What do you think about caching the compiled content here ?

What is the status of this ??

JHyun abandoned this revision.Jun 27 2016, 9:32 PM

This patch is too old.