Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Stefan Monnier]

> @@ -1465,10 +1466,10 @@
>         contains 32 elements, and each element covers 128 characters.  A
>         sub char-table of depth 3 contains 128 elements, and each element
>         is for one character.  */
> -    Lisp_Object depth;
> +    int depth;
 
>      /* Minimum character covered by the sub char-table.  */
> -    Lisp_Object min_char;
> +    int min_char;
 
>      /* Use set_sub_char_table_contents to set this.  */
>      Lisp_Object contents[FLEXIBLE_ARRAY_MEMBER];
> @@ -1539,12 +1540,16 @@
>      const char *doc;
>    };
 
> -/* This is the number of slots that every char table must have.  This
> -   counts the ordinary slots and the top, defalt, parent, and purpose
> -   slots.  */
> -enum CHAR_TABLE_STANDARD_SLOTS
> +enum char_table_specials
>    {
> -    CHAR_TABLE_STANDARD_SLOTS = PSEUDOVECSIZE (struct Lisp_Char_Table, extras)
> +    /* This is the number of slots that every char table must have.  This
> +       counts the ordinary slots and the top, defalt, parent, and purpose
> +       slots.  */
> +    CHAR_TABLE_STANDARD_SLOTS = PSEUDOVECSIZE (struct Lisp_Char_Table, extras),
> +
> +    /* This is an index of first Lisp_Object field in Lisp_Sub_Char_Table
> +       when the latter is treated as an ordinary Lisp_Vector.  */
> +    SUB_CHAR_TABLE_OFFSET = PSEUDOVECSIZE (struct Lisp_Sub_Char_Table, contents)
>    };
 
Are we sure this SUB_CHAR_TABLE_OFFSET exists?  I don't see any reason
why `contents' should necessarily be aligned on a multiple of Lisp_Objects.
So, I think it's unsafe to cast your new "struct Lisp_Sub_Char_Table"
to a "struct Lisp_Vector".  Maybe it does work on current existing
systems, but it's too fragile.


        Stefan

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Dmitry Antipov]

On 07/02/2014 06:30 PM, Stefan Monnier wrote:

> Are we sure this SUB_CHAR_TABLE_OFFSET exists?  I don't see any reason
> why `contents' should necessarily be aligned on a multiple of Lisp_Objects.

Strictly speaking, you're right. ISO C99 says (6.7.2.1) just:

"Each non-bit-field member of a structure or union object is aligned in an
implementation-defined manner appropriate to its type."

In fact, on a 64-bit system and 32-bit system with --with-wide-int, sizeof (int) is
4 and sizeof (Lisp_Object) is 8. On a 64-bit system, any reasonable compiler should
pack 2 integers to 8-byte area, so SUB_CHAR_TABLE_OFFSET is 1; if the compiler is
stupid and packs each field to 8-byte slot, SUB_CHAR_TABLE_OFFSET is 2. On a 32-bit
system, SUB_CHAR_TABLE_OFFSET is 2 if sizeof (Lisp_Object) is 4 and 1 if
sizeof(Lisp_Object) is 8; anyway, it should be aligned (unless some packing tricks
are in effect?)

> So, I think it's unsafe to cast your new "struct Lisp_Sub_Char_Table"
> to a "struct Lisp_Vector".  Maybe it does work on current existing
> systems, but it's too fragile.

I'll install verify() to catch future (broken) systems.

Dmitry

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Stefan Monnier]

>> So, I think it's unsafe to cast your new "struct Lisp_Sub_Char_Table"
>> to a "struct Lisp_Vector".  Maybe it does work on current existing
>> systems, but it's too fragile.
> I'll install verify() to catch future (broken) systems.

The problem is not only future systems.  On systems where Lisp_Object
are 64bit but can be aligned on a 32bit boundary (I expect this to be
the case for some 32bit systems --with-wide-int), if/when we optimize
the representation a bit more (i.e. merge `depth' and `min_char' into
a single 32bit word, by adding ":2" and ":30" after each field), your
code will probably fail.


        Stefan

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Dmitry Antipov]

On 07/02/2014 07:42 PM, Stefan Monnier wrote:

>if/when we optimize
> the representation a bit more (i.e. merge `depth' and `min_char' into
> a single 32bit word, by adding ":2" and ":30" after each field), your
> code will probably fail.

I've considered this possible optimization too and rejected it exactly
for the reason you're talking about.

Dmitry

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Stefan Monnier]

>> if/when we optimize the representation a bit more (i.e. merge `depth'
>> and `min_char' into a single 32bit word, by adding ":2" and ":30"
>> after each field), your code will probably fail.
> I've considered this possible optimization too and rejected it exactly
> for the reason you're talking about.

But the problem is that the code is vulnerable to these kinds
of changes.  Why do we even need to treat such a Lisp_Sub_Char_Table as
a Lisp_Vector at all?  I don't see any real need for it.


        Stefan

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Dmitry Antipov]

On 07/02/2014 10:27 PM, Stefan Monnier wrote:

> But the problem is that the code is vulnerable to these kinds
> of changes.  Why do we even need to treat such a Lisp_Sub_Char_Table as
> a Lisp_Vector at all?  I don't see any real need for it.

This is somewhat similar to r110830 when we shrink struct vectorlike_header.
Sub char-table is not a marginal and almost-not-used type (and the latter was
a surprise to me) - for example, after C-h h I have more than 3K of live
sub char-tables. On a 64-bit system, saving just 8 byte per sub char-table
gives 24K. Not too much, but avoiding 2 Lisp_Objects also shaves off 6K calls
to mark_object at GC (these calls was useless anyway just because depth and
min_char are always integers and so don't need to be marked).

Of course, an universal Eli-style "we don't need nano-improvements" argument
makes all of the above just a nonsense.

Dmitry

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Stephen J. Turnbull]

Dmitry Antipov writes:

 > Of course, an universal Eli-style "we don't need nano-improvements" argument
 > makes all of the above just a nonsense.

Eli's been a little cranky, but he does have a point.

However, if you never have more than an hour to devote to Emacs at one
time, and typically those hours are spread far enough apart that your
short-term memory is guaranteed to time out, working on the ambiguous
problems of extending functionality to new areas is going to be
painful unless you're extremely good at picking up exactly where you
left off.  Optimizing existing functionality may be a better use of
your time.

Nevertheless, please consider Eli's line of argument carefully, and at
least don't make fun of it.

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Stefan Monnier]

>> But the problem is that the code is vulnerable to these kinds
>> of changes.  Why do we even need to treat such a Lisp_Sub_Char_Table as
>> a Lisp_Vector at all?  I don't see any real need for it.
> This is somewhat similar to r110830 when we shrink struct vectorlike_header.

I'm not arguing against your optimization.  While I don't think it's
important, I'm perfectly happy if you work on such things.

But I don't see why that necessarily implies casting
Lisp_Sub_Char_Table to a Lisp_Vector (I ask this question without
having spent much time looking for the answer in the source, maybe the
answer is obvious, but I think if you can tell me why it's a better use
of my time than trying to figure it out myself).


        Stefan

Re: Nano-improvements (was: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.)

[Eli Zaretskii]

> Date: Thu, 03 Jul 2014 08:12:14 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> Cc: emacs-devel@gnu.org
> 
> Of course, an universal Eli-style "we don't need nano-improvements" argument
> makes all of the above just a nonsense.

I didn't say "we don't need nano-improvements".  I said they should
take a backseat to more substantial improvements in functionality,
especially user-visible functionality.

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Eli Zaretskii]

> From: "Stephen J. Turnbull" <stephen@xemacs.org>
> Date: Thu, 03 Jul 2014 14:01:30 +0900
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> > Of course, an universal Eli-style "we don't need nano-improvements" argument
> > makes all of the above just a nonsense.
> 
> Eli's been a little cranky, but he does have a point.

Yes, cranky is my middle name, especially after reading too much stuff
that gets me irritated.  Too late to change that, so all I can say is
sorry, and try harder to keep my crankiness at bay.

> However, if you never have more than an hour to devote to Emacs at one
> time, and typically those hours are spread far enough apart that your
> short-term memory is guaranteed to time out, working on the ambiguous
> problems of extending functionality to new areas is going to be
> painful unless you're extremely good at picking up exactly where you
> left off.  Optimizing existing functionality may be a better use of
> your time.

There are simple devices to overcome those difficulties.  Like keeping
notes or recording easy-to-forget details in the commit logs of your
feature branch.  Having been there, I don't think this is such a big
problem, nor that it's the root cause for the proliferation of
"nano-improvements".

> Nevertheless, please consider Eli's line of argument carefully, and at
> least don't make fun of it.

Thank you.

I'm arguing that with the amount of developmental energy we have at
our disposal (an average of 250 commits per month lately, more than
4000 per last year!), I would expect the project to gain significant
new functionality much faster than what we see in etc/NEWS.  As a
simple thought experiment, I suggest to read etc/NEWS entries for the
last 2 or 3 releases and make a list of the important functionality
and usability changes you see there.  Then reflect on the number of
those changes.

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Dmitry Antipov]

On 07/03/2014 05:57 PM, Stefan Monnier wrote:

> But I don't see why that necessarily implies casting
> Lisp_Sub_Char_Table to a Lisp_Vector (I ask this question without
> having spent much time looking for the answer in the source, maybe the
> answer is obvious, but I think if you can tell me why it's a better use
> of my time than trying to figure it out myself).

Hm... look at mark_char_table in alloc.c - this function implies that
both Lisp_Char_Table and Lisp_Sub_Char_Table pointers can be treated
as a pointer to their base type Lisp_Vector. Having both mark_char_table
and mark_sub_char_table looks a bit overengineered for me. This also
applies to Lisp_Char_Table/Lisp_Sub_Char_Table printing code in
print_object, etc.

Dmitry

Re: Nano-improvements

[Dmitry Antipov]

On 07/03/2014 06:53 PM, Eli Zaretskii wrote:

> I didn't say "we don't need nano-improvements".  I said they should
> take a backseat to more substantial improvements in functionality,
> especially user-visible functionality.

IMO all of this friction just comes from the different views on what
Emacs is. If this is just an editor, you're absolutely correct. But if
someone treats it as a unique Lisp runtime system which can do a lot of
interesting things beyond evaluating s-expressions, an opposite
conclusions are much more likely. For example, what is more important -
a) ability to edit huge files without loading all data into memory or
b) JIT compiler from byte code to native code? If we're developing
just an editor, a) is much more important, at least to match the
proprietary competitors like SlickEdit; but if we're developing the
language for the specific problem domain (and want to extend this
domain as our language becomes more and more faster), b) can be a step
to a lot of impressive opportunities.

Dmitry

Re: Nano-improvements

[Eli Zaretskii]

> Date: Thu, 03 Jul 2014 19:58:44 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> On 07/03/2014 06:53 PM, Eli Zaretskii wrote:
> 
> > I didn't say "we don't need nano-improvements".  I said they should
> > take a backseat to more substantial improvements in functionality,
> > especially user-visible functionality.
> 
> IMO all of this friction just comes from the different views on what
> Emacs is.

I don't think so.  But if it's true, then perhaps we could benefit
from asking the project lead to express their vision of what are the
project goals, both near-term and long-term, and what are non-goals.

> If this is just an editor, you're absolutely correct. But if
> someone treats it as a unique Lisp runtime system which can do a lot of
> interesting things beyond evaluating s-expressions, an opposite
> conclusions are much more likely. For example, what is more important -
> a) ability to edit huge files without loading all data into memory or
> b) JIT compiler from byte code to native code? If we're developing
> just an editor, a) is much more important, at least to match the
> proprietary competitors like SlickEdit; but if we're developing the
> language for the specific problem domain (and want to extend this
> domain as our language becomes more and more faster), b) can be a step
> to a lot of impressive opportunities.

I think all of the above is important; they are by no means
"nano-improvements".

I'm saying that there are too few changes in any of the
above-mentioned areas (and then some).  So their relative importance
is not the issue, because they all currently eat dust, while most of
our energy is applied elsewhere.

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Dmitry Antipov]

On 07/03/2014 06:55 PM, Eli Zaretskii wrote:

> I'm arguing that with the amount of developmental energy we have at
> our disposal (an average of 250 commits per month lately, more than
> 4000 per last year!), I would expect the project to gain significant
> new functionality much faster than what we see in etc/NEWS.  As a
> simple thought experiment, I suggest to read etc/NEWS entries for the
> last 2 or 3 releases and make a list of the important functionality
> and usability changes you see there.  Then reflect on the number of
> those changes.

Large and mature software project is somewhat similar to an old castle.
Building new walls is very important for defense, but it doesn't make
too much sense if an internal passes are cluttered up so that soldiers
can't run through them and climb up walls in time.

Dmitry

Re: Nano-improvements C integers to Lisp_Objects.

[Eli Zaretskii]

> Date: Thu, 03 Jul 2014 20:12:06 +0400
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> Large and mature software project is somewhat similar to an old castle.
> Building new walls is very important for defense, but it doesn't make
> too much sense if an internal passes are cluttered up so that soldiers
> can't run through them and climb up walls in time.

Ah, but I'm thinking about going on offense, not defense.

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Stefan Monnier]

>> But I don't see why that necessarily implies casting
>> Lisp_Sub_Char_Table to a Lisp_Vector (I ask this question without
>> having spent much time looking for the answer in the source, maybe the
>> answer is obvious, but I think if you can tell me why it's a better use
>> of my time than trying to figure it out myself).

> Hm... look at mark_char_table in alloc.c - this function implies that
> both Lisp_Char_Table and Lisp_Sub_Char_Table pointers can be treated
> as a pointer to their base type Lisp_Vector. Having both mark_char_table
> and mark_sub_char_table looks a bit overengineered for me. This also
> applies to Lisp_Char_Table/Lisp_Sub_Char_Table printing code in
> print_object, etc.

Could you try to use something like the patch below to eliminate this
alignment assumption?


        Stefan


=== modified file 'src/alloc.c'
--- src/alloc.c	2014-07-02 03:26:19 +0000
+++ src/alloc.c	2014-07-03 16:47:25 +0000
@@ -5962,13 +5962,18 @@
 {
   int size = ptr->header.size & PSEUDOVECTOR_SIZE_MASK;
   /* Consult the Lisp_Sub_Char_Table layout before changing this.  */
-  int i, idx = (pvectype == PVEC_SUB_CHAR_TABLE ? SUB_CHAR_TABLE_OFFSET : 0);
+  int i;
+  Lisp_Object *contents
+    = pvectype == PVEC_SUB_CHAR_TABLE
+    ? (size -= SUB_CHAR_TABLE_OFFSET,
+       ((struct Lisp_Sub_Char_Table *)ptr)->contents)
+    : ((struct Lisp_Char_Table *)ptr)->contents;
 
   eassert (!VECTOR_MARKED_P (ptr));
   VECTOR_MARK (ptr);
-  for (i = idx; i < size; i++)
+  for (i = 0; i < size; i++)
     {
-      Lisp_Object val = ptr->contents[i];
+      Lisp_Object val = contents[i];
 
       if (INTEGERP (val) || (SYMBOLP (val) && XSYMBOL (val)->gcmarkbit))
 	continue;

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Dmitry Antipov]

On 07/03/2014 08:49 PM, Stefan Monnier wrote:

> Could you try to use something like the patch below to eliminate this
> alignment assumption?
>
> === modified file 'src/alloc.c'
> --- src/alloc.c	2014-07-02 03:26:19 +0000
> +++ src/alloc.c	2014-07-03 16:47:25 +0000
> @@ -5962,13 +5962,18 @@
>   {
>     int size = ptr->header.size & PSEUDOVECTOR_SIZE_MASK;
>     /* Consult the Lisp_Sub_Char_Table layout before changing this.  */
> -  int i, idx = (pvectype == PVEC_SUB_CHAR_TABLE ? SUB_CHAR_TABLE_OFFSET : 0);
> +  int i;
> +  Lisp_Object *contents
> +    = pvectype == PVEC_SUB_CHAR_TABLE
> +    ? (size -= SUB_CHAR_TABLE_OFFSET,
> +       ((struct Lisp_Sub_Char_Table *)ptr)->contents)
> +    : ((struct Lisp_Char_Table *)ptr)->contents;

This is obviously wrong because Lisp_Object slots of Lisp_Char_Table
between 'header' and 'contents' are never marked. This code should
looks like:

   Lisp_Object *contents
     = pvectype == PVEC_SUB_CHAR_TABLE
     ? (size -= SUB_CHAR_TABLE_OFFSET,
        ((struct Lisp_Sub_Char_Table *)ptr)->contents)
     : ptr->contents;


And, IIUC, 'size -= SUB_CHAR_TABLE_OFFSET' still assumes that
SUB_CHAR_TABLE_OFFSET is a multiple of sizeof (Lisp_Object)
(otherwise subtracting it from size, which is a number of
Lisp_Object slots, makes no sense).

Dmitry

Re: [Emacs-diffs] trunk r117464: Shrink Lisp_Sub_Char_Table by preferring C integers to Lisp_Objects.

[Stefan Monnier]

>> -  int i, idx = (pvectype == PVEC_SUB_CHAR_TABLE ? SUB_CHAR_TABLE_OFFSET : 0);
>> +  int i;
>> +  Lisp_Object *contents
>> +    = pvectype == PVEC_SUB_CHAR_TABLE
>> +    ? (size -= SUB_CHAR_TABLE_OFFSET,
>> +       ((struct Lisp_Sub_Char_Table *)ptr)->contents)
>> +    : ((struct Lisp_Char_Table *)ptr)->contents;

> This is obviously wrong because Lisp_Object slots of Lisp_Char_Table
> between 'header' and 'contents' are never marked.

The patch was just meant to show the intention.  I didn't try to make
it correct.

>   Lisp_Object *contents
>     = pvectype == PVEC_SUB_CHAR_TABLE
>     ? (size -= SUB_CHAR_TABLE_OFFSET,
>        ((struct Lisp_Sub_Char_Table *)ptr)->contents)
>     : ptr->contents;


> And, IIUC, 'size -= SUB_CHAR_TABLE_OFFSET' still assumes that
> SUB_CHAR_TABLE_OFFSET is a multiple of sizeof (Lisp_Object)
> (otherwise subtracting it from size, which is a number of
> Lisp_Object slots, makes no sense).

No, SUB_CHAR_TABLE_OFFSET is not supposed to be a multiple of anything,
neither in my proposal nor in the current code.


        Stefan