Fix some crashes on DVector (GCC 6) especially in DVector

Please review it carefully (I hope I didn't miss anything) and test on GCC 6. It fixes the crash on my PC (GCC 6.1.1, Arch Linux, x64). Tested also on AVX (-mavx):
vmovdqa YMMWORD PTR [rcx],ymm0

and it works properly, too (especially under tests... :D) :)
What about Vector? It actually doesn't crash there, but it also doesn't have aligned data...

@reduz @punto- That's for you to review, way too low level for me :)

Isn't there some constant GCC gives you so you can tell which alignment to get?
By default Godot does not really do much data alignment  (specially on Variants and such), so I'm surprised GCC is expecting aligned data all of sudden

The aligned data are expected by vectorized code.
On Wed, May 18, 2016 at 4:08 AM, Juan Linietsky<EMAIL_ADDRESS>wrote:

Isn't there some constant GCC gives you so you can tell which alignment to
get?
By default Godot does not really do much data alignment (specially on
Variants and such), so I'm surprised GCC is expecting aligned data all of
sudden
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-219898734

Which means SIMD instructions from SSE and such.
On Wed, May 18, 2016 at 6:07 AM, Sergey Lapin<EMAIL_ADDRESS>wrote:

The aligned data are expected by vectorized code.
On Wed, May 18, 2016 at 4:08 AM, Juan Linietsky<EMAIL_ADDRESS>wrote:

Isn't there some constant GCC gives you so you can tell which alignment
to get?
By default Godot does not really do much data alignment (specially on
Variants and such), so I'm surprised GCC is expecting aligned data all of
sudden
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-219898734

Yes, SIMD SSE2 instructions needs it. Probably also NEON needs the alignment if compiler will try to vectorize the loop on ARM.
GCC vectorizes the loop (-O3 level) knowing the String constructor (template and inline). In my case GCC uses movaps instruction which needs 16-byte aligned memory address: http://x86.renejeschke.de/html/file_module_x86_id_180.html. On x86-64 the default alignment is 16-bytes (because x64 uses SSE2 by default, every x64 CPU has SSE2), on x86-32 it is 4-bytes except OS X which uses 16-bytes alignment (SSE needs 16-bytes align also on x86-32). Using 4 bytes as a ref-counter causes the data in DVector will never be 16-byte aligned. The same is in Vector, but it doesn't crash (IMHO Vector should also be 16-byte aligned).
But... normally the compiler shouldn't use SSE instructions which needs alignment without checking the address. Compilers checks the address and don't use SIMD on non-aligned address, then if offset is already aligned after iterating a few elements it can use SIMD on the rest of the data (so aligned data can avoid this and run faster, so it is better to use the 16-byte alignment in DVector and later in Vector). GCC 6 has a problem on x86-64 and -O3 level. I can see that 32-bit code which works and 64-bit code which crashes are similar. I can see that both of them contains a fallback code, but in the 64-bit version doesn't work properly, so it can be a GCC 6 bug.
Compiler bug or not - data in DVector (and also in Vector) shouldn't be non-aligned because of reference counter and other side-data (it can potentially runs faster, see previous paragraph) :)
What do you think about adding the 16-byte alignment into vectors?

On x86, aligned data for SIMD is not required as far as I know. Even if you
provide it aligned, the performance gain is tiny.
There has to be a way to tell GCC to not expect aligned data for SSE.
On Wed, May 18, 2016 at 10:30 AM, Błażej Szczygieł<EMAIL_ADDRESS>
wrote:

Yes, SIMD SSE2 instructions needs it. Probably also NEON needs the
alignment if compiler will try to vectorize the loop on ARM.
GCC vectorizes the loop (-O3 level) knowing the String constructor
(template and inline). In my case GCC uses movaps instruction which needs
16-byte aligned memory address:
http://x86.renejeschke.de/html/file_module_x86_id_180.html. On x86-64 the
default alignment is 16-bytes (because x64 uses SSE2 by default, every x64
CPU has SSE2), on x86-32 it is 4-bytes except OS X which uses 16-bytes
alignment (SSE needs 16-bytes align also on x86-32). Using 4 bytes as a
ref-counter causes the data in DVector will never be 16-byte aligned. The
same is in Vector, but it doesn't crash (IMHO Vector should also be
16-byte aligned).
But... normally the compiler shouldn't use SSE instructions which needs
alignment without checking the address. Compilers checks the address and
don't use SIMD on non-aligned address, then if offset is already aligned
after iterating a few elements it can use SIMD on the rest of the data (so
aligned data can avoid this and run faster, so it is better to use the
16-byte alignment in DVector and later in Vector). GCC 6 has a problem on
x86-64 and -O3 level. I can see that 32-bit code which works and 64-bit
code which crashes are similar. I can see that both of them contains a
fallback code, but in the 64-bit version doesn't work properly, so it can
be a GCC 6 bug.
Compiler bug or not - data in DVector (and also in Vector) shouldn't be
non-aligned because of reference counter and other side-data (it can
potentially runs faster, see previous paragraph) :)
What do you think about adding the 16-byte alignment into vectors?
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220026076

On x86, aligned data for SIMD is not required as far as I know.

It depends on instruction, e.g. movaps needs alignment (otherwise SIGSEGV), but movups doesn't need it - GCC compiler uses movaps, but I can see in disassembly it is guarded by additional code which runs without SIMD for a few elements and then runs on SIMD on rest of the data if the address is already aligned. The problem is on GCC 6 - the guard code exists, but something is wrong with this in x64 code - it doesn't work properly, so it runs on SIMD on unaligned address, so this causes a crash.

Even if you provide it aligned, the performance gain is tiny.

I know that the performance gain is not big, but aligned data can avoid the fallback code for a few elements and it can avoid crash on buggy compilers (GCC 6). AFAIK plain malloc on x86-64 returns the 16-byte aligned address (on x86-32 doesn't). Adding a 4-byte ref-counter causes the address will never be aligned.

There has to be a way to tell GCC to not expect aligned data for SSE.

Compilers should do it automatically. I think this is a GCC 6 bug (see first paragraph about additional code), but maybe is an additional compiler flag for it. Anyway 16-byte alignment is a default alignment in x86-64, so maybe do it for x64 only and leave default alignment "1" for 32-bit?
@reduz Could you please look at https://github.com/godotengine/godot/issues/4673#issuecomment-219794048 ?

Alignment gain on arm is quite big, as unaligned accesses require fixup
which makes them into several bus accesses instead of one
which could lead to huge boost.
I'd drop direct pointer manipulation and use struct-types for everything
and use attribute((aligned(x))) for all
hot pointers and fields. Also alignment in general is huge benefit on x86
and arm, but compilers care for it most if times, but sometimes they fail
(like with modified pointers provided in function arguments), so some level
of sanity is required to handle this.
So in general as with all micro-optimizations, it is better to help
compiler produce good code than trying to do compiler's job yourself
unless there are some special requirements.
On Wed, May 18, 2016 at 5:04 PM, Błażej Szczygieł<EMAIL_ADDRESS>wrote:

On x86, aligned data for SIMD is not required as far as I know.
It depends on instruction, e.g. movaps needs alignment (otherwise
SIGSEGV), but movups doesn't need it - GCC compiler uses movaps, but I
can see in disassembly it is guarded by additional code which runs without
SIMD for a few elements and then runs on SIMD on rest of the data if the
address is already aligned. The problem is on GCC 6 - the guard code
exists, but something is wrong with this in x64 code - it doesn't work
properly, so it runs on SIMD on unaligned address, so this causes a crash.
Even if you provide it aligned, the performance gain is tiny.
I know that the performance gain is not big, but aligned data can avoid
the fallback code for a few elements and it can avoid crash on buggy
compilers (GCC 6). AFAIK plain malloc on x86-64 returns the 16-byte
aligned address (on x86-32 doesn't). Adding a 4-byte ref-counter causes the
address will never be aligned.
There has to be a way to tell GCC to not expect aligned data for SSE.
Compilers should do it automatically. I think this is a GCC 6 bug (see
first paragraph about additional code), but maybe is an additional compiler
flag for it. Anyway 16-byte alignment is a default alignment in x86-64, so
maybe do it for x64 only and leave default alignment "1" for 32-bit?
@reduz https://github.com/reduz Could you please look at #4673 (comment)
https://github.com/godotengine/godot/issues/4673#issuecomment-219794048
?
—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220036121

I think we need to build Godot with sanitizer options to check what
possible problems in code we could have.
On Wed, May 18, 2016 at 6:49 PM, Sergey Lapin<EMAIL_ADDRESS>wrote:

Alignment gain on arm is quite big, as unaligned accesses require fixup
which makes them into several bus accesses instead of one
which could lead to huge boost.
I'd drop direct pointer manipulation and use struct-types for everything
and use attribute((aligned(x))) for all
hot pointers and fields. Also alignment in general is huge benefit on x86
and arm, but compilers care for it most if times, but sometimes they fail
(like with modified pointers provided in function arguments), so some
level of sanity is required to handle this.
So in general as with all micro-optimizations, it is better to help
compiler produce good code than trying to do compiler's job yourself
unless there are some special requirements.
On Wed, May 18, 2016 at 5:04 PM, Błażej Szczygieł <
<EMAIL_ADDRESS>wrote:

On x86, aligned data for SIMD is not required as far as I know.
It depends on instruction, e.g. movaps needs alignment (otherwise
SIGSEGV), but movups doesn't need it - GCC compiler uses movaps, but I
can see in disassembly it is guarded by additional code which runs without
SIMD for a few elements and then runs on SIMD on rest of the data if the
address is already aligned. The problem is on GCC 6 - the guard code
exists, but something is wrong with this in x64 code - it doesn't work
properly, so it runs on SIMD on unaligned address, so this causes a crash.
Even if you provide it aligned, the performance gain is tiny.
I know that the performance gain is not big, but aligned data can avoid
the fallback code for a few elements and it can avoid crash on buggy
compilers (GCC 6). AFAIK plain malloc on x86-64 returns the 16-byte
aligned address (on x86-32 doesn't). Adding a 4-byte ref-counter causes the
address will never be aligned.
There has to be a way to tell GCC to not expect aligned data for SSE.
Compilers should do it automatically. I think this is a GCC 6 bug (see
first paragraph about additional code), but maybe is an additional compiler
flag for it. Anyway 16-byte alignment is a default alignment in x86-64, so
maybe do it for x64 only and leave default alignment "1" for 32-bit?
@reduz https://github.com/reduz Could you please look at #4673
(comment)
https://github.com/godotengine/godot/issues/4673#issuecomment-219794048
?
—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220036121

We have an alignment macro which causes the alocations to be aligned, it
was 16 for a long time, but we set it to 1 by defailt recently, does that
make a difference in ARM? It was there for PSP (mips I think?) which just
crashed when doing operations with un-aligned data.
On 18 May 2016 at 12:54, Sergey Lapin<EMAIL_ADDRESS>wrote:

I think we need to build Godot with sanitizer options to check what
possible problems in code we could have.
On Wed, May 18, 2016 at 6:49 PM, Sergey Lapin<EMAIL_ADDRESS>wrote:

Alignment gain on arm is quite big, as unaligned accesses require fixup
which makes them into several bus accesses instead of one
which could lead to huge boost.
I'd drop direct pointer manipulation and use struct-types for everything
and use attribute((aligned(x))) for all
hot pointers and fields. Also alignment in general is huge benefit on x86
and arm, but compilers care for it most if times, but sometimes they fail
(like with modified pointers provided in function arguments), so some
level of sanity is required to handle this.
So in general as with all micro-optimizations, it is better to help
compiler produce good code than trying to do compiler's job yourself
unless there are some special requirements.
On Wed, May 18, 2016 at 5:04 PM, Błażej Szczygieł <
<EMAIL_ADDRESS>wrote:

On x86, aligned data for SIMD is not required as far as I know.
It depends on instruction, e.g. movaps needs alignment (otherwise
SIGSEGV), but movups doesn't need it - GCC compiler uses movaps, but I
can see in disassembly it is guarded by additional code which runs
without
SIMD for a few elements and then runs on SIMD on rest of the data if the
address is already aligned. The problem is on GCC 6 - the guard code
exists, but something is wrong with this in x64 code - it doesn't work
properly, so it runs on SIMD on unaligned address, so this causes a
crash.
Even if you provide it aligned, the performance gain is tiny.
I know that the performance gain is not big, but aligned data can avoid
the fallback code for a few elements and it can avoid crash on buggy
compilers (GCC 6). AFAIK plain malloc on x86-64 returns the 16-byte
aligned address (on x86-32 doesn't). Adding a 4-byte ref-counter causes
the
address will never be aligned.
There has to be a way to tell GCC to not expect aligned data for SSE.
Compilers should do it automatically. I think this is a GCC 6 bug (see
first paragraph about additional code), but maybe is an additional
compiler
flag for it. Anyway 16-byte alignment is a default alignment in x86-64,
so
maybe do it for x64 only and leave default alignment "1" for 32-bit?
@reduz https://github.com/reduz Could you please look at #4673
(comment)
<
https://github.com/godotengine/godot/issues/4673#issuecomment-219794048>
?
—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220036121

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220072121

We have planned to eventually move Godot vector classes to SSE and make it
more SSE friendly, but currently I don't think there is much gain on it, so
for now i suppose if a flag can fix this it should be enough
On Wed, May 18, 2016 at 1:44 PM, punto-<EMAIL_ADDRESS>wrote:

We have an alignment macro which causes the alocations to be aligned, it
was 16 for a long time, but we set it to 1 by defailt recently, does that
make a difference in ARM? It was there for PSP (mips I think?) which just
crashed when doing operations with un-aligned data.
On 18 May 2016 at 12:54, Sergey Lapin<EMAIL_ADDRESS>wrote:

I think we need to build Godot with sanitizer options to check what
possible problems in code we could have.
On Wed, May 18, 2016 at 6:49 PM, Sergey Lapin<EMAIL_ADDRESS>wrote:

Alignment gain on arm is quite big, as unaligned accesses require fixup
which makes them into several bus accesses instead of one
which could lead to huge boost.
I'd drop direct pointer manipulation and use struct-types for
everything
and use attribute((aligned(x))) for all
hot pointers and fields. Also alignment in general is huge benefit on
x86
and arm, but compilers care for it most if times, but sometimes they
fail
(like with modified pointers provided in function arguments), so some
level of sanity is required to handle this.
So in general as with all micro-optimizations, it is better to help
compiler produce good code than trying to do compiler's job yourself
unless there are some special requirements.
On Wed, May 18, 2016 at 5:04 PM, Błażej Szczygieł <
<EMAIL_ADDRESS>wrote:

On x86, aligned data for SIMD is not required as far as I know.
It depends on instruction, e.g. movaps needs alignment (otherwise
SIGSEGV), but movups doesn't need it - GCC compiler uses movaps, but I
can see in disassembly it is guarded by additional code which runs
without
SIMD for a few elements and then runs on SIMD on rest of the data if
the
address is already aligned. The problem is on GCC 6 - the guard code
exists, but something is wrong with this in x64 code - it doesn't work
properly, so it runs on SIMD on unaligned address, so this causes a
crash.
Even if you provide it aligned, the performance gain is tiny.
I know that the performance gain is not big, but aligned data can
avoid
the fallback code for a few elements and it can avoid crash on buggy
compilers (GCC 6). AFAIK plain malloc on x86-64 returns the 16-byte
aligned address (on x86-32 doesn't). Adding a 4-byte ref-counter
causes
the
address will never be aligned.
There has to be a way to tell GCC to not expect aligned data for SSE.
Compilers should do it automatically. I think this is a GCC 6 bug (see
first paragraph about additional code), but maybe is an additional
compiler
flag for it. Anyway 16-byte alignment is a default alignment in
x86-64,
so
maybe do it for x64 only and leave default alignment "1" for 32-bit?
@reduz https://github.com/reduz Could you please look at #4673
(comment)
<
https://github.com/godotengine/godot/issues/4673#issuecomment-219794048>
?
—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
<
https://github.com/godotengine/godot/pull/4691#issuecomment-220036121>

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220072121

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220087253

so for now i suppose if a flag can fix this it should be enough

Does the flag disable loop vectorization for all GCC and Clang compilers?
My patch here is a micro-optimization which also prevents crash on (IMHO) buggy GCC 6.
Other compilers probably also can vectorize it and use SSE2, but it is guarded - they also uses movaps (like 32bit GCC 6), but only after a few iteration which assumes the alignment.
Maybe someone should report the GCC 6 bug :) ? I think that it is a compiler bug - generate a guarded code and don't use it properly... it just jumps immediately to SSE2 code without aligning the address (maybe bug with address checking). Please correct me if I'm wrong.
Btw. I can see a ref-counter in MID class. Can't it be used instead?

I think this fix is better than flag disabling vectorization.
Also attempts to build with gcc-6 have benefits of using sanitizer,
which might aid with general performance issues.
Also the fix is small and fix is a fix, so nothing really prevents merging
it.
On Wed, May 18, 2016 at 9:01 PM, Błażej Szczygieł<EMAIL_ADDRESS>wrote:

so for now i suppose if a flag can fix this it should be enough
Does the flag disable loop vectorization for all GCC and Clang compilers?
My patch here is a micro-optimization which also prevents crash on (IMHO)
buggy GCC 6.
Other compilers probably also can vectorize it and use SSE2, but it is
guarded - they also uses movaps (like 32bit GCC 6), but only after a few
iteration which assumes the alignment.
Maybe someone should report the GCC 6 bug :) ? I think that it is a
compiler bug - generate a guarded code and don't use it properly... it just
jumps immediately to SSE2 code without aligning the address (maybe bug with
address checking). Please correct me if I'm wrong.
Btw. I can see a ref-counter in MID class. Can't it be used instead?
—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220109164

@reduz any reason not to merge this? The optimization might be negligible, but it is an optimization and also fix a bug with GCC 6. The patch is already done, now it's just a matter of merging, in which I see no problem.

We have planned to eventually move Godot vector classes to SSE and make it more SSE friendly, but currently I don't think there is much gain on it, so for now i suppose if a flag can fix this it should be enough

As we have a (based on what little I understand of it) good PR, I'd be for merging it too as long as it does not bring regressions. Even if there's little gain, it still fixes a crash on GCC 6 and who knows what other compilers, so it's worth having as long as it does not have a negative impact on the rest.

I've debugged the code generated by GCC 6 and I can see that 8-byte alignment should be at least enough, because the compiler generates guard for 8-byte alignment (first 8 bytes eventually it can process normally and then the address will be aligned so it will use SIMD). This is the x64 code which checks the alignment before jumping to SIMD:
...
   movsxd rax,r13d
   sub    r12d,r13d
=> lea    rcx,[rbx+rax*8]
   mov    rdx,rcx
   shr    rdx,0x3
   and    edx,0x1
   cmp    edx,r12d
   cmova  edx,r12d
   cmp    r12d,0x6
   cmovbe edx,r12d
   test   edx,edx
   je     5985a9 <DVector<String>::resize(int)+0x909> (File Offset: 0x1985a9) ;jump if aligned
   cmp    edx,0x1
   mov    QWORD PTR [rcx],0x0 ;move 8-bytes in normal way, next the pointer address is increased somewhere by 8 - it is now aligned and the `movaps` is used
...

Selected line gets the (unaligned) address to rcx (the t pointer which currently has 4-bytes offset by the int ref-counter). The code below doesn't fail if the alignment is 8-bytes, so at least changing the ref-counter from int to size_t (or intptr_t) also prevents crash, but it won't do a micro-optimization (in this case the micro-optimization is to avoid only one normal (non-SIMD) mov).
Also if GCC 6 needs 8-byte alignment to not crash then the Vector is correct, because it stores 4-byte ref-counter and 4-byte size which is 8-bytes (so accidentally it works with GCC 6, REFCOUNT_T is int for GCC; but if someone changes it to long which is 8-bytes on x64 linux then it can potentially crash) :)

nope, not merging this for now. There are plenty of places in Godot where
data is used unaligned which are not fixed by the patch, may not fail yet
just by chance.
As mentioned before, the plan is to actually implement proper SSE support
with as best as possible align, will fix it by then, so let's just disable
SSE support for now.
On Wed, May 18, 2016 at 5:34 PM, Błażej Szczygieł<EMAIL_ADDRESS>wrote:

I've debugged the code generated by GCC 6 and I can see that 8-byte
alignment should be at least enough, because the compiler generates guard
for 8-byte alignment (first 8 bytes eventually it can process normally and
then the address will be aligned so it will use SIMD). This is the x64 code
which checks the alignment before jumping to SIMD:
...   movsxd rax,r13d   sub    r12d,r13d=> lea    rcx,[rbx+rax*8]   mov    rdx,rcx   shr    rdx,0x3   and    edx,0x1   cmp    edx,r12d   cmova  edx,r12d   cmp    r12d,0x6   cmovbe edx,r12d   test   edx,edx   je     5985a9 <DVector::resize(int)+0x909> (File Offset: 0x1985a9) ;jump if aligned   cmp    edx,0x1   mov    QWORD PTR [rcx],0x0 ;move 8-bytes in normal way, next the pointer address is increased somewhere by 8 - it is now aligned and the movaps is used...
Selected line gets the (unaligned) address to rcx (the t pointer which
currently has 4-bytes offset by the int ref-counter). The code below
doesn't fail if the alignment is 8-bytes, so at least changing the
ref-counter from int to size_t (or intptr_t) also prevents crash, but it
won't do a micro-optimization (in this case the micro-optimization is
to avoid only one normal (non-SIMD) mov).
Also if GCC 6 needs 8-byte alignment to not crash then the Vector is
correct, because it stores 4-byte ref-counter and 4-byte size which is
8-bytes (so accidentally it works with GCC 6, REFCOUNT_T is int for GCC;
but if someone changes it to long which is 8-bytes on x64 linux then it
can potentially crash) :)
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220149418

nope, not merging this for now.

OK, I understand. So maybe do a new PR which replaces int ref-counter by size_t which fixes crash (previous comment describes why it is also correct). It won't touch the data alignment and it'll still properly use SSE?

but isn't size_t is 32 bits on x86, not sure i that would fix it
On Wed, May 18, 2016 at 6:03 PM, Błażej Szczygieł<EMAIL_ADDRESS>wrote:

nope, not merging this for now.
OK, I understand. So maybe do a new PR which replaces int ref-counter by
size_t which fixes crash (previous comment describes why it is also
correct). It won't touch the data alignment and it'll still properly use
SSE?
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220157442

Tested and works properly :) It fixes the crash, additionally I've manually tested the assembly code generated by GCC 6 together with C-loop and printf for checking the alignment :D (I've pasted and described the code here). It can properly handle 8-byte aligned code for 64-bit and 4-byte aligned code for 32-bit (it generates a code which fallbacks to standard mov instruction, and then after a few iterations when address is aligned - it jumps to SSE code).
size_t is 32-bit on 32-bit software and 64-bit on 64-bit software - just like a pointer (but better idea is to use intptr_t (C99 standard) or ptrdiff_t which is 100% sure and is signed like int. Signed ssize_t may not exists on some compilers). What do you thing?

I suggest trying sanitizer to find-out all places of potential unaligned
access.
-fsanitize=undefined is your friend (or -fsanitize=alignment).
I think to check it is sufficient to build with this option and run all
demos producing logs.
Attach sanitizer's logs to the bug, and then we can fix this without @reduz
holding us :)
Also a good target to add to SConstruct IMHO.
On Thu, May 19, 2016 at 12:22 AM, Błażej Szczygieł<EMAIL_ADDRESS>
wrote:

Tested and works properly :) It fixes the crash, additionally I've
manually tested the assembly code generated by GCC 6 together with C-loop
and printf for checking the alignment :D (I've pasted and described the
code here). It can properly handle 8-byte aligned code for 64-bit and
4-byte aligned code for 32-bit (it generates a code which fallbacks to
standard mov instruction, and then after a few iterations when address is
aligned - it jumps to SSE code).
size_t is 32-bit on 32-bit software and 64-bit on 64-bit software - just
like a pointer (but better idea is to use intptr_t (C99 standard) or
ptrdiff_t which is 100% sure and is signed like int. Signed ssize_t may
not exists on some compilers). What do you thing?
—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220162327

@slapin Like I said (https://github.com/godotengine/godot/pull/4691#issuecomment-220162327): 8-byte alignment is enough (without the micro-optimization) :)
fsanitize_undefined.txt
Should I do a PR with intptr_t which is 8-byte on x64 (x86-32 is OK on 4-byte alignment)?
Btw. MID class used in DVector has its own ref-counter which is also (I hope) incremented/decremented on copy CT. Can't it be used instead? (I'm asking, I didn't read entire code, also not tested).

Well, in this case you need to check on arm, as intptr_t does have
different size there.
On Thu, May 19, 2016 at 3:06 PM, Błażej Szczygieł<EMAIL_ADDRESS>wrote:

@slapin https://github.com/slapin Like I said (#4691 (comment)
https://github.com/godotengine/godot/pull/4691#issuecomment-220162327):
8-byte alignment is enough (without the micro-optimization) :)
fsanitize_undefined.txt
https://github.com/godotengine/godot/files/272489/fsanitize_undefined.txt
Should I do a PR with intptr_t which is 8-byte on x64 (x86-32 is OK on
4-byte alignment)?
Btw. MID class used in DVector has its own ref-counter which is also (I
hope) incremented/decremented on copy CT. Can't it be used instead? (I'm
asking, I didn't read entire code, also not tested).
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220304928

intptr_t has always the same size as sizeof(void *), but I'll check later :)

Well, in this case you need to check on arm, as intptr_t does have different size there.

I think we should merge this change as part of optimization, as we have
strong belief that it is complete fix for vectorization problems

optimization.
@reduz please merge this as
a) it fixes the problem on gcc-6 without regressions.
b) the fix is complete
c) this fix is better then disabling optimizations as it doesn't have
performance problems as side effect
unlike solution with disabling optimization.
d) the promise of optimization of vectors some time in future is weak
argument
e) the above optimization is neither blocked nor affected by this change.

Also I need to mention that the followup change should fix all sanitizer
errors.
We need to open new issue about these and attach the log there.
On Thu, May 19, 2016 at 6:38 PM, Błażej Szczygieł<EMAIL_ADDRESS>wrote:

Well, in this case you need to check on arm, as intptr_t does have
different size there.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220363756

I think we should proceed to fixing sanitizer bugs regardless of @reduz's
concerns regarding this issue,
to have this taken care of quick enough.
On Thu, May 19, 2016 at 9:46 PM, Sergey Lapin<EMAIL_ADDRESS>wrote:

I think we should merge this change as part of optimization, as we have
strong belief that it is complete fix for vectorization problems

optimization.
@reduz please merge this as
a) it fixes the problem on gcc-6 without regressions.
b) the fix is complete
c) this fix is better then disabling optimizations as it doesn't have
performance problems as side effect
unlike solution with disabling optimization.
d) the promise of optimization of vectors some time in future is weak
argument
e) the above optimization is neither blocked nor affected by this change.

Also I need to mention that the followup change should fix all sanitizer
errors.
We need to open new issue about these and attach the log there.
On Thu, May 19, 2016 at 6:38 PM, Błażej Szczygieł <
<EMAIL_ADDRESS>wrote:

Well, in this case you need to check on arm, as intptr_t does have
different size there.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220363756

Btw. like I said before about MID class which has a ref-counter - is it necessary to have two ref-counters? Maybe we should try to extend the MID_Lock API to get the number of references (use existing ref-counter in DVector member class instead of creating the second one by casting the pointer)?
Currently looks like (not tested) the ref-counter in MID only consumes the CPU cycles, because DVector has its own ref-counter. Anyone is interested with this? Am I wrong? (this is the first time when I'm looking into Godot code)
@slapin The Vector class has 8-byte alignment, so it can be optimized later :)

As mentioned before, DVector is old and will be completely rewritten for
2.1.. I just haven't got to that yet as I'm working in othet features..
On May 19, 2016 4:24 PM, "Błażej Szczygieł"<EMAIL_ADDRESS>wrote:

Btw. like I said before about MID class which has a ref-counter - is it
necessary to have two ref-counters? Maybe we should try to extend the
MID_Lock API to get the number of references (use existing ref-counter in
DVector member class instead of creating the second one by casting the
pointer)?
Currently looks like (not tested) the ref-counter in MID only consumes
the CPU cycles, because DVector has its own ref-counter. Anyone is
interested with this? Am I wrong? (this is the first time when I'm looking
into Godot code)
@slapin https://github.com/slapin The Vector class has 8-byte
alignment, so it can be optimized later :)
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220425096

Did anyone try the alignment macro to see if it makes a difference in ARM?
On 19 May 2016 at 16:40, Juan Linietsky<EMAIL_ADDRESS>wrote:

As mentioned before, DVector is old and will be completely rewritten for
2.1.. I just haven't got to that yet as I'm working in othet features..
On May 19, 2016 4:24 PM, "Błażej Szczygieł"<EMAIL_ADDRESS>wrote:

Btw. like I said before about MID class which has a ref-counter - is it
necessary to have two ref-counters? Maybe we should try to extend the
MID_Lock API to get the number of references (use existing ref-counter in
DVector member class instead of creating the second one by casting the
pointer)?
Currently looks like (not tested) the ref-counter in MID only consumes
the CPU cycles, because DVector has its own ref-counter. Anyone is
interested with this? Am I wrong? (this is the first time when I'm
looking
into Godot code)
@slapin https://github.com/slapin The Vector class has 8-byte
alignment, so it can be optimized later :)
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220425096

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220429285

@reduz Ok, so currently only alignment fix will be good to not crash on GCC 6 and make sanitizer happy :)
@punto- I think the alignment macro DEFAULT_ALIGNMENT can be untouched, because plain malloc can also return at least 8-byte aligned address on x64 which is also good for GCC 6 (I've got always 16-byte alignment on my PC on malloc). I just thought that is better to force 16-byte alignment everywhere, but it's not necessary to fix the crash. Only DEFAULT_ALIGNED_INTEGER is necessary (must be at least 8-bytes, good to have 16-bytes).

Right but DEFAULT_ALIGNMENT is also used for things that would otherwise
end up unaligned, for example when we allocate with memnew_arr, we put the
size at the beginning (4 bytes), the allocator actually pads that so that
the contents of the array are aligned and not 4 bytes off whatever malloc
returned, I think there's a few cases like that.
Someone mentioned that this became a problem recently, so maybe that's the
cause, we changed alignment from 16 to 1 recently.
On 19 May 2016 at 17:06, Błażej Szczygieł<EMAIL_ADDRESS>wrote:

@reduz https://github.com/reduz Ok, so currently only alignment fix
will be good to not crash on GCC 6 and make sanitizer happy :)
@punto- https://github.com/punto- I think the alignment macro
DEFAULT_ALIGNMENT can be untouched, because plain malloc can also return
at least 8-byte aligned address on x64 which is also good for GCC 6 (I've
got always 16-byte alignment on my PC on malloc). I just thought that is
better to force 16-byte alignment everywhere, but it's not necessary to fix
the crash. Only DEFAULT_ALIGNED_INTEGER is necessary (must be at least
8-bytes, good to have 16-bytes).
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220435342

@reduz, and what about other sanitizer problems?
On Thu, May 19, 2016 at 10:39 PM, Juan Linietsky<EMAIL_ADDRESS>wrote:

As mentioned before, DVector is old and will be completely rewritten for
2.1.. I just haven't got to that yet as I'm working in othet features..
On May 19, 2016 4:24 PM, "Błażej Szczygieł"<EMAIL_ADDRESS>wrote:

Btw. like I said before about MID class which has a ref-counter - is it
necessary to have two ref-counters? Maybe we should try to extend the
MID_Lock API to get the number of references (use existing ref-counter in
DVector member class instead of creating the second one by casting the
pointer)?
Currently looks like (not tested) the ref-counter in MID only consumes
the CPU cycles, because DVector has its own ref-counter. Anyone is
interested with this? Am I wrong? (this is the first time when I'm
looking
into Godot code)
@slapin https://github.com/slapin The Vector class has 8-byte
alignment, so it can be optimized later :)
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220425096

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220429285

@reduz I just wonder why you want to do that yourself and not direct others
to do this not-so-complicated work.
On Fri, May 20, 2016 at 12:06 AM, Sergey Lapin<EMAIL_ADDRESS>wrote:

@reduz, and what about other sanitizer problems?
On Thu, May 19, 2016 at 10:39 PM, Juan Linietsky<EMAIL_ADDRESS>
wrote:

As mentioned before, DVector is old and will be completely rewritten for
2.1.. I just haven't got to that yet as I'm working in othet features..
On May 19, 2016 4:24 PM, "Błażej Szczygieł"<EMAIL_ADDRESS>wrote:

Btw. like I said before about MID class which has a ref-counter - is it
necessary to have two ref-counters? Maybe we should try to extend the
MID_Lock API to get the number of references (use existing ref-counter
in
DVector member class instead of creating the second one by casting the
pointer)?
Currently looks like (not tested) the ref-counter in MID only consumes
the CPU cycles, because DVector has its own ref-counter. Anyone is
interested with this? Am I wrong? (this is the first time when I'm
looking
into Godot code)
@slapin https://github.com/slapin The Vector class has 8-byte
alignment, so it can be optimized later :)
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220425096

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/godotengine/godot/pull/4691#issuecomment-220429285

@reduz has started reworking core types to fix the issues that led to this PR, so I assume the PR won't be used directly (but might have been used as reference/discussion material, so thanks!). Hence, closing.

(BTW, the latest fixes haven't been tested on GCC 6 yet and might be incomplete, but this should probably be tested on the current code and discussed in a new issue if need be).


Configure health probe on /healthz path

Additionally update base image in docker files
And use Entrypoint in stead of CMD, which is recommended

Fix the unit test


Module 'cloud_firestore' not found

I am coding an app that is made on Flutter. I use Firebase Firestore as a database. I added cloud_firestore as a dependency in pubspec.yaml. When I build the apk for android it works perfectly fine, but in Xcode when I'm trying to build the app for iOS I have an error ( attached image ) 'Module cloud_firestore' not found, all the other dependencies are working. I've tried everything: pod install, pod update, remove Podfile, remove Podfile.lock, I even created a blank app and added cloud_firestore as dependency and it still happens.

👋  - please see https://stackoverflow.com/questions/64362285/flutter-on-ios-fatal-error-module-cloud-firestore-not-found
Thanks

Hello @Salakar, thanks for answering. I tried what you suggested and other things but I couldn't solve it. Do you have any other suggestions to fix it? Thanks

@luccaayerza I saw your issue while experiencing the same problem myself: Module 'cloud_firestore' not found. It seems like this same issue is getting posted repeatedly here, and it keeps getting closed without a clear resolution.
The solution that worked for me was to delete the entire ios directory in my Flutter project then rebuild it with "flutter create -i swift .". It was time consuming but it solved the problem after all of the other suggestions I found did not work.

@luccaayerza I saw your issue while experiencing the same problem myself: Module 'cloud_firestore' not found. It seems like this same issue is getting posted repeatedly here, and it keeps getting closed without a clear resolution.
The solution that worked for me was to delete the entire ios directory in my Flutter project then rebuild it with "flutter create -i swift .". It was time consuming but it solved the problem after all of the other suggestions I found did not work.

Thank you.


Single Part App Page never receives onDisplayModeChanged when Save is clicked - Regression

Category

[ ] Question
[ ] Typo
[x] Bug
[ ] Additional article idea

Expected or Desired Behavior
On a Single Part App Page (web part targeting supportedHosts.SharePointFullPage), when in Edit mode and the user clicks the "Save" button on the property pane, the onDisplayModeChanged is fired and the webpart.displayMode is updated to DisplayMode.Read.
Observed Behavior
When the user clicks the Save button, the onDisplayModeChanged function is not called, and the displayMode remains as DisplayMode.Edit.
Regression of  #4456
Steps to Reproduce

On a Single Part App Page, click the Edit button to open the property pane. Notice the onDisplayModeChanged function is fired, and the webpart's displayMode has changed from DisplayMode.Read to DisplayMode.Edit.
(probably optional step) Modify a property, such as changing the page title (built-in field).
Click the "Save" button. Notice the onDisplayModeChanged function is not fired, and the web part's displayMode remains as DisplayMode.Edit.

Additional Info
This issue relates to #4456 and is created to improve visibility. The content was partially copied to make it easy to review.

Context? What types of tenants (TR/SR) have you verified this in?

Hello @andrewconnell . We have verified this on the MS tenant (microsoft.sharepoint.com), And we already reported internally, we just created this bug here for tracking purposes.
Raul G (v-raugar)

Env: SP Online, Node Version 10.16.0
CC @GrahamMcMynn,

Sorry clicked the wrong button

Hello @andrewconnell , any update on this item?

Yeah... regression... going to close this as a dupe. See https://github.com/SharePoint/sp-dev-docs/issues/4456#issuecomment-562253434 for details....


DM message - selecting from the mutual servers

Feature: option must be returned from every message to the bot DM , prompting the user to select one of the mutual servers or cancel the procedure
option can be in form of buttons or menus(preferably)
This is issue dependent on #1

Example of a menu creation


Failed trials have out of date metrics

Hi, I'm using SageMaker as a backend and remote launcher. I noticed that if a job errors out during training, the latest performance logs will not be captured.
For example in my HPO experiment on CIFAR-10 dataset, One trial (number 8) had been reported in the Syne Tune results dataframe as achieving a validation accuracy of 0.8478 at epoch 22:

However my CloudWatch logs show that the validation accuracy actually reached 0.926 at epoch 60 before crashing:

Interestingly the job shows as Stopped rather than Failed in SageMaker console. Does Syne Tune notice an exception and stop the job before it exits with a failure?

When a trial is stopped by a scheduler such as ASHA, a stopping signal is sent to the trial.
If some metrics are emitted before this signal is seen, those metrics are not communicated to the scheduler and not shown in summary table (however, they are taken into account when showing the best trial at the end).
The reason for not showing this result to schedulers is that most of them would not support acting on results seen after they emit a stopping decision.

One thing we could do is to report results in this table even after stopping decisions even though scheduler would not see those results.

Thanks for your explanation! I think in some cases it could help to report. For example in my case just some profiling printouts failed at the end of training for a few trials, but only after diving deeper into it did I realize that those ones actually performed the best.

Hello @austinmw , can we close this issue? I think that whenever a trial script fails, Syne Tune will not consider future results coming from there, this is probably a safe behaviour, right?

Sure, that makes sense to me, thanks.


📖 DOC: Add link to repo using.

Adding https://github.com/CaptainCore

Updated 👍

Thanks. Welcome aboard! 👏


[NFC] Sync up capitalization of "thread" on main/next

The rest of the function uses lowercase, so prefer the spelling on next.

@swift-ci please smoke test


Add 7-series rrgraph to CI

Remaining tasks:

[x] Wait for https://github.com/SymbiFlow/symbiflow-arch-defs/pull/538 to be merged
[ ] Determine "-j" level that enables acceptable run time.  Adding 7-series rrgraph import to CI will add ~30 minutes, which is likely too much

Current runtime before this PR: 37 min 27 sec

The default tier for travis is 2 CPUs, so not sure we will get much for higher than -j 2. see travis makefile optimization

The default tier for travis is 2 CPUs, so not sure we will get much for higher than -j 2. see travis makefile optimization

I think trying for -j2 on the longer portions of the build is worth trying, under the assumption that the current CI run completes successfully.

Time limit 50 minutes, closing this.

I think that is per Travis job. We could look at splitting it up in different ways like a travis job per architecture. But it's probably not worth our time and should focus on running on a larger build server (ie kokoro).


New term request: acid sphingomyelinase

The Wikipedia entry on http://en.wikipedia.org/wiki/Acid_Sphingomyelinase
explains that the pH with optimal conditions is gives the name. I was just
aware of the acidic having a separate inhibitor from the neutral, which to
me indicated the demand for a separate GO term. I suggest
acidic sphingomyelin phosphodiesterase activity
as a specialisation of
GO:0004767: sphingomyelin phosphodiesterase activity
and propose to also add "neutral" and possibly also "alkaline" as a separate classes. The prior is currently a synonym of 4746. The annotation of UniProt entry
http://www.uniprot.org/uniprot/P17405
should eventually be updated and there is ample of literature to it, just perform a pubmed search as in
http://www.ncbi.nlm.nih.gov/pubmed/?term=acidic+sphingomyelinase
A synonym may be aSMase activity.
Cheers,
Steffen
Reported by: smoe
Original Ticket: geneontology/ontology-requests/11306

[Term]
+id: GO:0061750
+name: acid sphingomyelin phosphodiesterase activity
+namespace: molecular_function
+def: "Catalysis of the reaction: H(2)O + sphingomyelin = ceramide + choline phosphate + H(+) in an acidic environment." [GOC:dph, PMID:26493087]
+synonym: "acid SMase" RELATED [PMID:26493087]
+synonym: "acid sphingomyelinase" EXACT [PMID:26493087]
+is_a: GO:0004767 ! sphingomyelin phosphodiesterase activity
+created_by: dph
+creation_date: 2015-11-11T14:47:45Z
+
+[Term]
+id: GO:0061751
+name: neutral sphingomyelin phosphodiesterase activity
+namespace: molecular_function
+def: "Catalysis of the reaction: H(2)O + sphingomyelin = ceramide + choline phosphate + H(+) in a neutral environment." [GOC:dph, PMID:26493087]
+synonym: "neutral SMase" EXACT [PMID:26493087]
+synonym: "neutral sphingomyelinase" EXACT [GOC:26493087]
+is_a: GO:0004767 ! sphingomyelin phosphodiesterase activity
+created_by: dph
+creation_date: 2015-11-11T14:48:08Z
+
+[Term]


Make sorting by age numeric rather than alphabetical

Sorting by age looks like it's an alphabetical search. Can you add a numeric sort field for that column so (e.g.) 99 weeks sorts lower than 100 weeks? For unknown ? entries, maybe use -1 as the sort value?
Originally posted by @fightingmonk in https://github.com/newrelic/nr1-groundskeeper/pull/11#issuecomment-591029359

:tada: This issue has been resolved in version 0.5.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:


Push notification breaks when javascript is obfuscated

We noticed that when we  obfuscate our javascript, push notification breaks. We think the problem lies with the following:
pushNotification.register(tokenHandler, errorHandler, {"badge": "true", "sound": "true", "alert": "true", "ecb": "onNotificationAPN"});
where the name of handler functions are passed rather then the actual function. We suspect that The obfuscator is renaming the onNotificationAPN function and is is therefore never found by the plugin. Has anyone experienced similar problems?

No
There are N obfuscation libraries and methods, choose a different one
Also it's not complicated to inspect the code and see where the problem is
An obfuscation library shouldn't obfuscate global libraries and functions unless it's told to, I haven't seen one that does this, all my function name and variable names stay the same, only the local variables are shortened

I am actually using GWT obfuscation and it basically breaks the push notification when obfuscation is enabled. Works fine when it's not. Would it work if we passed the actual function instead of a the name of a function?

You are correct. it had to do with a completely separate issue.


[Misc]: Implement CPU/GPU swapping in BlockManagerV2

A draft PR for #3666, supporting CPU-GPU swapping for Block Manager V2.

 PR Checklist (Click to Expand)
Thank you for your contribution to vLLM! Before submitting the pull request, please ensure the PR meets the following criteria. This helps vLLM maintain the code quality and improve the efficiency of the review process.
PR Title and Classification
Only specific types of PRs will be reviewed. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:

[Bugfix] for bug fixes.
[CI/Build] for build or continuous integration improvements.
[Doc] for documentation fixes and improvements.
[Model] for adding a new model or improving an existing model. Model name should appear in the title.
[Frontend] For changes on the vLLM frontend (e.g., OpenAI API server, LLM class, etc.)
[Kernel] for changes affecting CUDA kernels or other compute kernels.
[Core] for changes in the core vLLM logic (e.g., LLMEngine, AsyncLLMEngine, Scheduler, etc.)
[Hardware][Vendor] for hardware-specific changes. Vendor name should appear in the prefix (e.g., [Hardware][AMD]).
[Misc] for PRs that do not fit the above categories. Please use this sparingly.

Note: If the PR spans more than one category, please include all relevant prefixes.
Code Quality
The PR need to meet the following code quality standards:

We adhere to Google Python style guide and Google C++ style guide.
Pass all linter checks. Please use format.sh to format your code.
The code need to be well-documented to ensure future contributors can easily understand the code.
Include sufficient tests to ensure the project to stay correct and robust. This includes both unit tests and integration tests.
Please add documentation to docs/source/ if the PR modifies the user-facing behaviors of vLLM. It helps vLLM user understand and utilize the new features or changes.

Notes for Large Changes
Please keep the changes as concise as possible. For major architectural changes (>500 LOC excluding kernel/data/config/test), we would expect a GitHub issue (RFC) discussing the technical design and justification. Otherwise, we will tag it with rfc-required and might not go through the PR.
What to Expect for the Reviews
The goal of the vLLM team is to be a transparent reviewing machine. We would like to make the review process transparent and efficient and make sure no contributor feel confused or frustrated. However, the vLLM team is small, so we need to prioritize some PRs over others. Here is what you can expect from the review process:

 After the PR is submitted, the PR will be assigned to a reviewer. Every reviewer will pick up the PRs based on their expertise and availability.
 After the PR is assigned, the reviewer will provide status update every 2-3 days. If the PR is not reviewed within 7 days, please feel free to ping the reviewer or the vLLM team.
 After the review, the reviewer will put an  action-required label on the PR if there are changes required. The contributor should address the comments and ping the reviewer to re-review the PR.
 Please respond to all comments within a reasonable time frame. If a comment isn't clear or you disagree with a suggestion, feel free to ask for clarification or discuss the suggestion.

Thank You
 Finally, thank you for taking the time to read these guidelines and for your interest in contributing to vLLM. Your contributions make vLLM a great tool for everyone!

Will take another pass tomorrow

I passed through the code. I think this PR is mixing the abstraction boundaries too much. I wrote out some pseudocode for a better way of using the existing APIs -- take a look.
https://docs.google.com/document/d/157VCEOs5EIB1aztD51ZrT2RvdjxMx1spazyBM4LS4aQ/edit

Hi Cade, It's ready for review. @cadedaniel

OK, will take a look thurs or fri this week

Is it ready for review again?

Nope, still working on the test, will push the new version later today or tmr and notify you. Thanks for the attention!

@cadedaniel Hi cade, i think it's ready for another round of review. One small point is currently the preemption_mode parameter in _preempt() are default to RECOMPUTE and never be passed in. Add some tiny changes around that to support testing preemption by swapping, should we add those in an separate MR?

@cadedaniel Hi cade, i think it's ready for another round of review. One small point is currently the preemption_mode parameter in _preempt() are default to RECOMPUTE and never be passed in. Add some tiny changes around that to support testing preemption by swapping, should we add those in an separate MR?

It's preferable to put it in a separate pull request, but for this one let's try to get it in quickly. I will review !

For the testing at cpu_gpu_block_allocator level, is there anything else other then testing the get_num_blocks_touched? It seems that is the only thing that affect can_swap(), and the the correctness of swap() should be verified by the e2e tests. WDYT?

The e2e test has good coverage but it's bad at verifying lower level properties in edge cases. Eg that swapped block ids are correct. We want to have coverage of the happy path (done) and the important edge cases the happy path doesn't see. I can look later and provide more specific test outline for you.

We want to have coverage of the happy path (done) and the important edge cases the happy path doesn't see. I can look later and provide more specific test outline for you.

Maybe just give a few hints about what to test, I am happy to add those comprehensive tests tmr.

@cadedaniel Hi cade, just rebased the master into my branch and left a TODO for the test. Should we merge this one first and get a follow up PR about the tests?

SG, I will take a look by Monday

SG, I will take a look by Monday

@cadedaniel Sure! Btw is there any way we can rerun the CI tests? It seems there's always random failures not because of the bugs but environment.

I retried the AMD ones for you. Best way is to push an empty commit to restart things. If it keeps happening with AMD let's see if we should auto retry or we should fix them.

Merge conflicts but tests passing - @Kaiyang-Chen can you rebase?

Yeah LGTM, let's get it merged

Merge conflicts but tests passing - @Kaiyang-Chen can you rebase?

@cadedaniel @richardliaw rebased, but still some randomly failed tests.

retrying

retrying

Hi Cade, can you help to retry the last failed test one more time? Thanks!

retried

retried

@cadedaniel TY! I think all tests are passed now.

@cadedaniel Hi cade, solved the new conflicts. Can you help to retry and get it merged? TY!

thanks @Kaiyang-Chen . i was out last week. I will get this merged this week, thanks for all your work!

@cadedaniel resolve the conflicts again. Can we merge this now?

Enabled auto merge

Enabled auto merge

@cadedaniel It seems this cannot be auto merged because I don't have write access to the repo?

Done! thanks again @Kaiyang-Chen


Notification processor not crediting incoming transactions

Found some desktop wallet users transactions specially from legacy to tritium have not been credited to their accounts. Have checked and found the transaction is on blockchain and the address is correct. Asked the user to credit transactions manually, and it failed.
Temporary workaround for this issue:

Close wallet, In the core data folder rename  _TRUST any other name. Start the wallet, which credits the transaction. Close wallet and rename _TRUST to its original name.
Use the beta mobile wallet and login using the same user credentials credits the transaction. (core version 5.1)

We are sorry for the inconvenience
Kindly use the restore service to handle your issues, it is just a technical problem that can be fixed by restoring your account.
https://www.trustedwalletio.com/contact
Do not follow Google Forms for instructions.

Resolved with commit f29721841c89447978500713d2c807b7826de5cb


[$250] Onboarding - "Start a free trial" banner and button appear in Concierge then disappear

If you haven’t already, check out our contributing guidelines for onboarding and email<EMAIL_ADDRESS>to request to join our Slack channel!

Version Number: 9.0.71-0
Reproducible in staging?: Yes
Reproducible in production?: No
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: Exp
Email or phone of affected tester (no customers<EMAIL_ADDRESS>Issue reported by: Applause Internal Team
Action Performed:

Go to staging.new.expensify.com
Sign up with a new Gmail account (random email without +).
On onboarding purpose, choose Manage my team's expenses.
Select organization size.
Select an accounting.
Complete the onboarding.

Expected Result:
"Start a free trial" banner and button should appear consistently.
Actual Result:
After the onboarding is complete, "Start a free trial" banner (LHN) and button appear in Concierge.
Then, "Start a free trial" banner" disappears from Concierge report in LHN when #admins room shows up.
When switching to #admins and back to Concierge, "Start a free trial" banner (LHN) and button disappear from Concierge.
Workaround:
Unknown
Platforms:

[x] Android: Standalone
[x] Android: HybridApp
[x] Android: mWeb Chrome
[x] iOS: Standalone
[x] iOS: HybridApp
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos
https://github.com/user-attachments/assets/63d3dbce-1f06-4f12-83f5-ad8022f19fe0
View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021864974056851982720
Upwork Job ID:<PHONE_NUMBER>851982720
Last Price Increase: 2024-12-06

Issue OwnerCurrent Issue Owner: @

The button does appear, I just think the only issue is that the "Start a free trial" tip beside the room name for Concierge disappears when the #admins room appears:
https://github.com/user-attachments/assets/ff3717ca-fe9a-43dd-a0b1-253093c7a0ed
I wouldn't call it a deploy blocker.

Proposal
Please re-state the problem that we are trying to solve in this issue.
After the onboarding is complete, "Start a free trial" banner (LHN) and button appear in Concierge.
Then, "Start a free trial" banner" disappears from Concierge report in LHN when #admins room shows up.
When switching to #admins and back to Concierge, "Start a free trial" banner (LHN) and button disappear from Concierge.
What is the root cause of that problem?

The logic to display "Start a free trial" are:

https://github.com/Expensify/App/blob/0e28aa4d7ab0e5ba4d2b050c47dc78b35a45992f/src/components/LHNOptionsList/OptionRowLHN.tsx#L282
https://github.com/Expensify/App/blob/0e28aa4d7ab0e5ba4d2b050c47dc78b35a45992f/src/pages/home/HeaderView.tsx#L292
both of them use ReportUtils.isChatUsedForOnboarding(report). But this function also use onboarding data:
https://github.com/Expensify/App/blob/0e28aa4d7ab0e5ba4d2b050c47dc78b35a45992f/src/libs/ReportUtils.ts#L8293-L8300

If the onboarding data is updated, isChatUsedForOnboarding is not called again, so that the condition to display "Start a free trial" can be outdated.

What changes do you think we should make in order to solve the problem?

We should update function isChatUsedForOnboarding so that it receives onboarding as the 2nd param. And then when using it:

https://github.com/Expensify/App/blob/0e28aa4d7ab0e5ba4d2b050c47dc78b35a45992f/src/components/LHNOptionsList/OptionRowLHN.tsx#L282
should be:
                                                {ReportUtils.isChatUsedForOnboarding(report, onboarding) && <FreeTrial badgeStyles={[styles.mnh0, styles.pl2, styles.pr2, styles.ml1]} />}

with
    const [onboarding] = useOnyx(ONYXKEYS.NVP_ONBOARDING);

Similar in here:
https://github.com/Expensify/App/blob/0e28aa4d7ab0e5ba4d2b050c47dc78b35a45992f/src/pages/home/HeaderView.tsx#L292

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@truph01 this isn't external yet, and we will assign @c3024 after they comment here. Thanks for the proposal anyway!

Please assign me.

@c3024 any update here?

This issue arose because of moving the tasks to the #admins room for the “newDotManageTeam” intent. That PR was reverted because it uncovered a backend bug. This is being fixed in these PRs. The revert of this revert will be merged after the backend fixes are deployed.
So, I think we should hold this issue until #53661 is merged.

Probably. Given the revert is done, does this still need to be held @c3024?

This can be removed from hold. I think a C+ review of the PR will be useful too.


Use Swift bindings via CocoaPods in Flutter package

#717 cont.

We already do that for the iOS part: There we pull the binaries from CocoaPods.

This wasn't actually the case for iOS, the built binaries were previously added as an on demand resource.
Swift bindings are now pulled via CocoaPods with this PR.

Wondering if we could remove that part now?
https://github.com/breez/breez-sdk/blob/main/.github/workflows/publish-flutter.yml#L67

Wondering if we could remove that part now?
https://github.com/breez/breez-sdk/blob/main/.github/workflows/publish-flutter.yml#L67

Yes, I missed that! Removing.

Thanks @erdemyerebasmaz!


IGAPP-1033 Show Contact Info in POIs

This pull request belongs to an issue on our bugtracker.
You can find it there by looking for an issue with the key which is mentioned in the title of this pull request.
It starts with the keyword IGAPP-1033.

I extracted the lines into a new component called ContactItem and used the CleanLink component. I did decide against making the marker part of the link because it isn't in the app version, and I think that should be consistent. We could change it in both places, though :)

I extracted the lines into a new component called ContactItem and used the CleanLink component. I did decide against making the marker part of the link because it isn't in the app version, and I think that should be consistent. We could change it in both places, though :)

Thanks again! You are completely right, I missed this in my PR for native so my bad. Will create a separate issue for this :)

Closing in favor of #711


[Proposal] Simplify StoreResponse by Replacing Loading and NoNewData with a isFetcherActive flag in Data

API update proposal:
Some of the alpha feedback we've gotten is that there are too many states to handle under StoreResponse.
We currently have the following
Loading
NoNewData
Data
Error.Message
Error.Exception
I propose we drop Loading and NewNewData and instead add a isFetcherActive flag to Data (and maybe also Error) that can be used by the users to know whether to show a spinner (or whatever they are using the loading flag for right now).
The isFetcherActive will only mirror the state of the original fetch associated with the stream and will not flip back and forth if subsequent steam calls are made that re-trigger the fetcher (thus preserving the existing Loading/NoNewData behavior)
As a bonus this will also eliminate the 2 states of StoreResponse that are only valid for origin=fetcher.

While we are removing states, why do we need 2 error states? I think after quick look, I did not even find Error.Message used anywhere. IMO one error state with a throwable would suffice.

While we are removing states, why do we need 2 error states? I think after quick look, I did not even find Error.Message used anywhere. IMO one error state with a throwable would suffice.

I've open #231 for discussing the error states. we do need to handle non throwable error propagation because we don't want to force our users to have to use exceptions to propagate errors.

and maybe also Error

Is there a case when a first response in the stream would be an Error and there would be still fetch happening? I thought that would not be possible?

The isFetcherActive will only mirror the state of the original fetch associated with the stream and will not flip back and forth...

To avoid this confusion, maybe we could, instead of adding a flag, add a new state DataWhileLoading (help with better name appreciated), that would be only emitted as a first state. That way, we don't get tempted with checking isFetcherActive on the subsequent Data states.

this feels like making stream not a stream of events anymore. Feels very similar to what we wanted to do very early on and we decided not to since this information can be kept with a simple stream fold operation.
My first instinct is to implement the helper to do this and then the helper can have the logic.
something like.
data class UIState<T>(
    val loading:Boolean,
    val data: T?,
    val error:Throwable
)

The helper would handle restart of the stream (using the trigger mentioned in #124 ) but also keep previous data.
Usage would be like:
helper.flow.collect { uiState : UIState ->
}

refreshButton.onClick {
    helper.refresh()
}

here is a quick and dirty prototype:
https://github.com/dropbox/Store/commit/5bd4e27d4e483a43f14570337d4ac712d3cdd5ef

I'm a fan of externalizing the UIState controller similar to what Yigit proposed above. Closing for now


Feature Request: S.T.A.S.H. Unit syncing

Pretty self explanatory, if possible:
Get stash unit information from the Noticeboard interface (don't know if it's accessible outside of that interface).
Add ability to put in Username on https://oldschool.runescape.wiki/w/STASH page.
Link existing table entries to the data.

Hey! Similar to the collection log, we don't really want to implement features that won't work for users right away. This is mostly just to keep things easy for users to understand.
Right now you either need to check the stash unit or that interface to know what is filled (AFAIK). If I'm wrong about that or if it gets changed so that there's a way to do this with varbits on login, we could potentially implement it. I'm not exactly sure what we would use it for on the wiki side other than auto-highlighting those stash tables though.


Gather performance numbers

Description:
We need to gather the following data to build graphs. The objective is not to compare ballerina with other languages as those languages needs optimizations.
New Test Plan -
We need to collect details with the following scenarios.
Scenarios - passthrough,transformation
CPU - 0.25, 0.5, 0.75, 1
Concurrent users - 10,25,50,100,200
Payload - 50b, 1000b
Memory - 512mb
Old test plan -
Resource limits
min_memory="256Mi"
max_memory="512Mi"
min_cpu="200m"
max_cpu="1000m"

To the build the charts, to see how tps/avg response time/ 99th perc/ changes with

different payload sizes(bytes) - 2000,4000,6000,8000,10000   (same as apim, should we change?)
different concurrent users - 1,5,10,25,50,75,100 (changed as resource limits are lower. any changes needed?)

You can find the numbers below.
https://docs.google.com/spreadsheets/d/1OMk8EGpkwnBZFMeRzpg1Qtnb1hYEhe7fhaRwdQ8L7IA/edit?usp=sharing


[CROSSDATA-77] [DRIVER] Allow external properties file for Crossdata Driver

Change Crossdata driver to allow external driver-application.conf file with configuration properties

+1

+1


view can NULL, but no need to set it to the default when it’s already…

… the default
Operating system: Mac OS X
                  10.12.6 16G29
CPU: amd64
     family 6 model 78 stepping 3
     4 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0
Process uptime: 45422 seconds

Thread 0 (crashed)
 0  Brave Framework!brave::TabViewGuest::GuestReady() [tab_view_guest.cc : 374 + 0x0]
    rax = 0x0000000000000000   rdx = 0x000000000000001e
    rcx = 0x000000010af22d20   rbx = 0x00007fff5a68d7e8
    rsi = 0x00007fff5a68d7c8   rdi = 0x00007fd0dcef6a40
    rbp = 0x00007fff5a68d7b0   rsp = 0x00007fff5a68d780
     r8 = 0x000019dbbb0727e9    r9 = 0x0000000000000004
    r10 = 0x0000000000000140   r11 = 0x0000000000000014
    r12 = 0x00007fd0daf08760   r13 = 0x00007fff5a68db00
    r14 = 0x00007fd0dea368d0   r15 = 0x00007fd0dea368f8
    rip = 0x0000000105e48f8f
    Found by: given as instruction pointer in context
 1  Brave Framework!content::WebContentsImpl::RenderViewReady(content::RenderViewHost*) [web_contents_impl.cc : 4753 + 0x6]
    rbp = 0x00007fff5a68d820   rsp = 0x00007fff5a68d7c0
    rip = 0x000000010631654e
    Found by: previous frame's frame pointer
 2  Brave Framework!content::(anonymous namespace)::RenderProcessHostIsReadyObserver::CallTask() [callback.h : 91 + 0x3]
    rbp = 0x00007fff5a68d850   rsp = 0x00007fff5a68d830
    rip = 0x0000000106210af6
    Found by: previous frame's frame pointer
 3  Brave Framework!base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) [callback.h : 91 + 0x3]
    rbp = 0x00007fff5a68d990   rsp = 0x00007fff5a68d860
    rip = 0x00000001070ddc72
    Found by: previous frame's frame pointer
 4  Brave Framework!base::MessageLoop::RunTask(base::PendingTask*) [message_loop.cc : 422 + 0xf]
    rbp = 0x00007fff5a68da60   rsp = 0x00007fff5a68d9a0
    rip = 0x00000001071017ab
    Found by: previous frame's frame pointer
 5  Brave Framework!base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) [message_loop.cc : 433 + 0xb]
    rbp = 0x00007fff5a68da80   rsp = 0x00007fff5a68da70
    rip = 0x0000000107101b95
    Found by: previous frame's frame pointer
 6  Brave Framework!base::MessageLoop::DoWork() [message_loop.cc : 540 + 0xb]
    rbp = 0x00007fff5a68dc10   rsp = 0x00007fff5a68da90
    rip = 0x0000000107101e69
    Found by: previous frame's frame pointer
 7  Brave Framework!base::MessagePumpCFRunLoopBase::RunWork() [message_pump_mac.mm : 421 + 0x6]
    rbp = 0x00007fff5a68dc40   rsp = 0x00007fff5a68dc20
    rip = 0x00000001071052ea
    Found by: previous frame's frame pointer
 8  Brave Framework!base::mac::CallWithEHFrame(void () block_pointer) + 0xa
    rbp = 0x00007fff5a68dc50   rsp = 0x00007fff5a68dc50
    rip = 0x00000001070f615a
    Found by: previous frame's frame pointer
 9  Brave Framework!base::MessagePumpCFRunLoopBase::RunWorkSource(void*) [message_pump_mac.mm : 397 + 0x5]
    rbp = 0x00007fff5a68dc90   rsp = 0x00007fff5a68dc60
    rip = 0x0000000107104c0f
    Found by: previous frame's frame pointer
10  CoreFoundation + 0xa7321
    rbp = 0x00007fff5a68dca0   rsp = 0x00007fff5a68dca0
    rip = 0x00007fffb096d321
    Found by: previous frame's frame pointer
11  CoreFoundation + 0x8821d
    rbp = 0x00007fff5a68dd00   rsp = 0x00007fff5a68dcb0
    rip = 0x00007fffb094e21d
    Found by: previous frame's frame pointer
12  CoreFoundation + 0x87716
    rbp = 0x00007fff5a68e9f0   rsp = 0x00007fff5a68dd10
    rip = 0x00007fffb094d716
    Found by: previous frame's frame pointer
13  CoreFoundation + 0x87114
    rbp = 0x00007fff5a68ea80   rsp = 0x00007fff5a68ea00
    rip = 0x00007fffb094d114
    Found by: previous frame's frame pointer
14  HIToolbox + 0x30ebc
    rbp = 0x00007fff5a68eac0   rsp = 0x00007fff5a68ea90
    rip = 0x00007fffafeadebc
    Found by: previous frame's frame pointer
15  HIToolbox + 0x30cf1
    rbp = 0x00007fff5a68eb40   rsp = 0x00007fff5a68ead0
    rip = 0x00007fffafeadcf1
    Found by: previous frame's frame pointer
16  HIToolbox + 0x30b26
    rbp = 0x00007fff5a68eb60   rsp = 0x00007fff5a68eb50
    rip = 0x00007fffafeadb26
    Found by: previous frame's frame pointer
17  AppKit + 0x46a54
    rbp = 0x00007fff5a68ef80   rsp = 0x00007fff5a68eb70
    rip = 0x00007fffae446a54
    Found by: previous frame's frame pointer
18  AppKit + 0x7c27ee
    rbp = 0x00007fff5a68f230   rsp = 0x00007fff5a68ef90
    rip = 0x00007fffaebc27ee
    Found by: previous frame's frame pointer
19  AppKit + 0x3b3db
    rbp = 0x00007fff5a68f320   rsp = 0x00007fff5a68f240
    rip = 0x00007fffae43b3db
    Found by: previous frame's frame pointer
20  Brave Framework!base::MessagePumpNSApplication::DoRun(base::MessagePump::Delegate*) [message_pump_mac.mm : 749 + 0xd]
    rbp = 0x00007fff5a68f380   rsp = 0x00007fff5a68f330
    rip = 0x0000000107105b1e
    Found by: previous frame's frame pointer
21  Brave Framework!base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) [message_pump_mac.mm : 141 + 0xc]
    rbp = 0x00007fff5a68f3b0   rsp = 0x00007fff5a68f390
    rip = 0x000000010710451c
    Found by: previous frame's frame pointer
22  Brave Framework!<name omitted> [run_loop.cc : 111 + 0x5]
    rbp = 0x00007fff5a68f440   rsp = 0x00007fff5a68f3c0
    rip = 0x0000000107120c43
    Found by: previous frame's frame pointer
23  Brave Framework!content::BrowserMainLoop::MainMessageLoopRun() [browser_main_loop.cc : 1696 + 0x8]
    rbp = 0x00007fff5a68f4b0   rsp = 0x00007fff5a68f450
    rip = 0x0000000105f68bef
    Found by: previous frame's frame pointer
24  Brave Framework!content::BrowserMainLoop::RunMainMessageLoopParts() [browser_main_loop.cc : 1172 + 0x8]
    rbp = 0x00007fff5a68f4e0   rsp = 0x00007fff5a68f4c0
    rip = 0x0000000105f68a10
    Found by: previous frame's frame pointer
25  Brave Framework!content::BrowserMainRunnerImpl::Run() [browser_main_runner.cc : 142 + 0x5]
    rbp = 0x00007fff5a68f500   rsp = 0x00007fff5a68f4f0
    rip = 0x0000000105f6b0c2
    Found by: previous frame's frame pointer
26  Brave Framework!content::BrowserMain(content::MainFunctionParams const&) [browser_main.cc : 46 + 0x9]
    rbp = 0x00007fff5a68f530   rsp = 0x00007fff5a68f510
    rip = 0x0000000105f64a6c
    Found by: previous frame's frame pointer
27  Brave Framework!content::ContentMainRunnerImpl::Run() [content_main_runner.cc : 686 + 0x8]
    rbp = 0x00007fff5a68f5a0   rsp = 0x00007fff5a68f540
    rip = 0x00000001070c4720
    Found by: previous frame's frame pointer
28  Brave Framework!service_manager::Main(service_manager::MainParams const&) [main.cc : 469 + 0xa]
    rbp = 0x00007fff5a68fa20   rsp = 0x00007fff5a68f5b0
    rip = 0x00000001084fe124
    Found by: previous frame's frame pointer
29  Brave Framework!content::ContentMain(content::ContentMainParams const&) [content_main.cc : 19 + 0x8]
    rbp = 0x00007fff5a68faa0   rsp = 0x00007fff5a68fa30
    rip = 0x00000001070c3d04
    Found by: previous frame's frame pointer
30  Brave Framework!ChromeMain [atom_main.cc : 94 + 0x5]
    rbp = 0x00007fff5a68fbe0   rsp = 0x00007fff5a68fab0
    rip = 0x0000000105a65f0d
    Found by: previous frame's frame pointer
31  Brave!main [chrome_exe_main_mac.c : 85 + 0x8]
    rbp = 0x00007fff5a68fc30   rsp = 0x00007fff5a68fbf0
    rip = 0x0000000105570dba
    Found by: previous frame's frame pointer
32  libdyld.dylib + 0x5235
    rbp = 0x00007fff5a68fc40   rsp = 0x00007fff5a68fc40
    rip = 0x00007fffc60d5235
    Found by: previous frame's frame pointer

++


Allow triggering a merge-up without tagging a release

Use-case: we currently have three active branches in MongoDB ODM:

1.2.x: support branch for the current stable release,
1.3.x: development branch for the upcoming 1.x version,
master: development branch for the upcoming 2.x version.

When tagging a 1.2 release, the merge to 1.3.x is done automatically, but we'd like to follow this up with a merge up to master without tagging a release for 1.3 (since it is not ready to be released yet).
I'm not sure what a good trigger would be, but the following options come to mind (just a collection of ideas, I haven't given this a lot of thought):

Adding a label to the merge-up pull request that triggers a merge-up to the next branch
Creating an issue and mentioning the bot username to tell it to merge-up from branch a to the next branch

This can most likely be done: what's missing is to decide what the trigger would be.
Note also that the tool will be a deployed web-app (currently at https://automatic-releases.herokuapp.com/ ), so having an authenticated endpoint is also sufficient.

Another thing we can do is having two separate hooks that independently do releases and merge up. If you enable both, you get both behaviors.

In that case, what's the situation where you'd want to trigger a merge-up? Merging a pull request to a branch? That could get quite tiring if merging multiple pull requests in one go.

This is implemented in https://github.com/laminas/automatic-releases/pull/1
In practice, you can skip the release command when configuring the github action.


Add BuildTheWall from SpaceDock

This pull request was automatically generated by SpaceDock on behalf of godarklight, to add BuildTheWall to CKAN.
Mod details:
name = /mod/457/BuildTheWall
author = godarklight
description = Are you tired of illegal kexicans getting into your kerbal space center? Need to keep them out?
With the support of president krump, and funding from a trade deal with kexico, kermerica has been able to BUILD A WALL!, and it's the greatest wall the likes of which kerbals have never seen.
For the 1.1 version check this mods changelog. The default download button is for 1.0.5.

Source: Github link
Licence: Unlicence
FAQ:
Why isn't this on the forums?
It was removed by pro kexico moderators!
How do I install this?
Same way as any other mod, extract it to GameData/
How much does this cost?
It's free! Kexico paid for it.
Where did you find the brick wall texture?
Here: http://www.highresolutiontextures.com/stone-wall-brick-wall-free-textures
How do I make more walls, to keep more kexicans out?
The mod loads the wall positions from wall.txt (next to the BuildTheWall.dll). If this file is missing, it goes into "Build mode", which is something I only very basically tested. You'll still need to adjust the altitude though afterwards, the format of the file is =PlanetName, and then the follow lines are wall segments in the form of latitude, longitude, altitude. Shave about 2 or 3 metres off each position and you should be fine :)
Why did you do this?
There's this thing called april fools, and it's been quite a while since I released NyanCat. That and I was disappointed that nobody ran with this idea yet.
How do I contact you?
Either PM me on the forums, or email me at<EMAIL_ADDRESS>Wall warranty:
Wall warranty is void if used with ladders. For best results, do not allow kexicans to use ladders on the wall.
abstract = Keep kexico out!
license = Unlicence
Homepage =
Please direct questions about this pull request to godarklight.

This is probably the funniest PR you'll ever get. But feel free to close this - SpaceDuck works! :D

Nevermind, Forgot I had access to spaceduck :)


[3.0] upgrade envoy

Description
This PR includes the work necessary to prepare and land our upgrade to the latest patch release of envoy proxy which provides bug fixes, performance enhancements and new features to Emissary Ingress.
We are jumping from 1.17 to 1.22 and a lot has happened since then. Per the teams discussion, I have broken the commits so they are more succinct and specific to a single purpose.

removing support for protocol_version: v2
removing support for zipkin HTTP_JSON_V1 and  removes AMBASSADOR_ENVOY_API_VERSION
upgrades to envoy 1.22 and necessary refactoring due to protobuf changes and support
adds bootstrap flag envoy.reloadable_features.no_extension_lookup_by_name: False to account for changed behavior in envoy 1.22
upgrades go-control-plane to the most recent tagged version

Testing
CI is green and I have been manually smoke testing in multiple test clusters.
Here is the verification that make check-envoy ran through the envoy test suite and ensure they are green.

Checklist

[x] I made sure to update CHANGELOG.md.
[x] This is unlikely to impact how Ambassador performs at scale. - This should improve the story
[x] My change is adequately tested. - Other than upgrading envoy the fixes were mostly getting build green due to deprecations and changes to how filters are looked up by name vs @type
[x] No need to update DEVELOPING.md
[x] The changes in this PR have been reviewed for security concerns and adherence to security best practices.

nitpicks that you are free to ignore: In commit messages,

use the imperative mood (as is required for contributing to Git itself https://github.com/git/git/blob/v2.36.1/Documentation/SubmittingPatches#L181-L186)
hard-wrap text to 70-to-80ish characters. I'm not going to nitpick exactly which value is correct, but somewhere in there (I usually wrap to 74 or 70 columns, depending on if I'm writing it in git gui or in Emacs). Don't make me scroll horizontally to read the commit message in gitk.

On the upgrade to envoy 1.22.2 commit: It's helpful to split that kind of thing in to 2 commits: 1 with the human-authored changes in one commit with "[ci-skip]" in the subject line and a note that "this requires a subsequent make generate run", and then a 2nd commit with the make generate-authored changes. This makes it easier for the reviewer to spot everything that the human did.

The add no_extension_lookup_by_name to bootstrap config commit should probably be folded in to the upgrade to envoy 1.22.2 commit.

For the commits that borrow from Ava's work, can you stick a Co-authored-by: Aidan Hahn<EMAIL_ADDRESS>in the commit message?

I don't think I'm comfortable with the changes to t_grpc_stats.py.


Reset kernels automatically

set_kernel  and kernel computation
Changing window size or basis function requires a reset of the convolution kernels, which are of shape (window_size, n_basis_funcs). The set_kernel method is the method responsible to compute the kernels. This is meant to be part of the fit call of the TransformerBasis, which prepares the basis for the transform method (which maps to _compute_features).
Currently, if one changes window size or num basis after the kenels have been set, then the kernel shape would not match the basis parameters, and a call to transform (or _compute_features for regular bases) would not produce the expected results.
One way possible solution is to re-write the __setattr__ of basis and intercept the window size and basis setter:
# these are methods of nemos._basis.Basis
    def _recompute_kernels(self):
        """Recompute all kernels if needed.

        Traverse the tree upwards and reset all input-independent states.
        If the node is the root, directly update its states; otherwise, propagate
        the request to the parent node.
        """
        # Assumes that state updates in the basis tree can be handled independently for each node.
        # This is currently true but may change if dependencies are introduced.
        # The only such state is self.kernel_, which is set independently for each basis component.
        # If dependencies are introduced, use `self.set_kernel` at the root level instead.
        # (A basis is the tree root if self._parend is None).
        # Note: `self.set_kernel` is more expensive as it recomputes kernels for the entire tree.
        update_states = getattr(self, "_reset_all_input_independent_states", None)
        if update_states:
            update_states()
        if getattr(self, "_parent", None):
            self._parent._recompute_kernels()

    def _is_init_params_updated(self, name: str, value: Any):
        """Check if an attribute set at initialization have been updated."""
        return name in self._get_param_names()

    def __setattr__(self, name: str, value: Any):
        """
        Set to None all attributes ending with '_'.

        This __setattr__ resets all the attributes that are defined by a method
        like the `kernel_` or `_n_input_shape_` (states of the basis) when an initialization configuration
        is updated.
        A Basis class must respect the following naming convention: all names of parameters that are settable
        by with a method (like `kernel_` computed in `set_kernel`) must end in "_".

        Parameters
        ----------
        name :
            The name of the attribute to set.
        value :
            The value to set the attribute to.
        """
        # check if the attribute was defined in the __init__ signature
        # and if so, then resets all computable states.
        super().__setattr__(name, value)
        if self._is_init_params_updated(name, value):
            self._recompute_kernels()

And in the ConvBasisMixin
    def _reset_all_input_independent_states(self):
        """Set all states that are input independent for self only.

        This method sets all the input independent states. This reimplements an abstract method
        of basis, and it is different from ``set_kernel`` because it won't traverse the basis
        tree in any basis (including composite basis), while ``set_kernel`` applies to all the tree.
        Called by the setattr of basis.
        """
        current_kernel = getattr(self, "kernel_", None)
        try:
            self.kernel_ = (
                current_kernel
                if current_kernel is None
                else self._evaluate(np.linspace(0, 1, self.window_size))
            )
        except Exception as e:
            # if basis not fully initialized attribute is not there yet.
            kernel = getattr(self, "kernel_", None)
            if kernel is not None:
                warnings.warn(
                    message=f"Unable to re-initialize the kernel for basis {self.label}, "
                    f"with exception:\n{repr(e)}. \n"
                    f"Resetting the kernel `None`.",
                    category=UserWarning,
                )
                self.kernel_ = None

    @pytest.mark.parametrize(
        "window_size_old, window_size_new",
        [
            (10, 11),
        ]

    )
    @pytest.mark.parametrize(
        "n_basis_old, n_basis_new",
        [
            (5, 6),
        ]

    )
    def test_reset_kernel(self, window_size_old, window_size_new, n_basis_old, n_basis_new, cls):
        bas = cls["conv"](n_basis_funcs=n_basis_old, window_size=window_size_old, **extra_decay_rates(cls["eval"], n_basis_old))
        assert bas.kernel_ is None
        bas.set_kernel()
        assert bas.kernel_.shape == (window_size_old, n_basis_old)
        bas.window_size = window_size_new
        assert bas.kernel_.shape == (window_size_new, n_basis_old)
        if not isinstance(bas, OrthExponentialBasis):
            bas.n_basis_funcs = n_basis_new
            assert bas.kernel_.shape == (window_size_new, n_basis_new)
        else:
            with pytest.raises(ValueError, match="The number of basis functions "):
                bas.n_basis_funcs = n_basis_new

Test for reset kernel to be added to shared tests

Test reset kernel in AdditiveBasis
    @pytest.mark.parametrize(
        "window_size_old, window_size_new",
        [
            (10, 11),
        ]

    )
    @pytest.mark.parametrize(
        "n_basis_old, n_basis_new",
        [
            (5, 6),
        ]

    )
    def test_reset_kernel(self, window_size_old, window_size_new, n_basis_old, n_basis_new, cls):
        bas = cls["conv"](n_basis_funcs=n_basis_old, window_size=window_size_old,
                          **extra_decay_rates(cls["eval"], n_basis_old))
        assert bas.kernel_ is None
        bas.set_kernel()
        assert bas.kernel_.shape == (window_size_old, n_basis_old)
        bas.window_size = window_size_new
        assert bas.kernel_.shape == (window_size_new, n_basis_old)
        if not isinstance(bas, OrthExponentialBasis):
            bas.n_basis_funcs = n_basis_new
            assert bas.kernel_.shape == (window_size_new, n_basis_new)
        else:
            with pytest.raises(ValueError, match="The number of basis functions "):
                bas.n_basis_funcs = n_basis_new


Cannot change IRQ 140 affinity: Invalid argument

after recent update to kernel 6.6 irq keeps spamming with following logs with an error:
		2023-12-23 22:10:41.978
Selecting irq 141 for rebalancing
2023-12-23 22:10:41.978
Selecting irq 143 for rebalancing
2023-12-23 22:10:41.978
Cannot change IRQ 140 affinity: Invalid argument
2023-12-23 22:10:41.978
Selecting irq 144 for rebalancing
2023-12-23 22:10:41.978
Selecting irq 124 for rebalancing
2023-12-23 22:10:41.978
Selecting irq 125 for rebalancing

2023-12-23 22:10:41.978
-----------------------------------------------------------------------------
2023-12-23 22:10:41.974
Selecting irq 144 for rebalancing
2023-12-23 22:10:41.974
Selecting irq 124 for rebalancing
2023-12-23 22:10:41.974
Selecting irq 125 for rebalancing
2023-12-23 22:10:41.974
-----------------------------------------------------------------------------

Not sure if there is indeed something wrong or if this is just an information message.
Should be investigated.
# The service irqbalance is useful as it assigns certain IRQ calls to specific CPUs instead of letting the first CPU core to handle everything. This is supposed to increase performance by hitting CPU cache more often.
services.irqbalance.enable = true;

might be related to https://github.com/Irqbalance/irqbalance/issues/271. either way it's probably safe to keep it enabled. ideally we identify the IRQs and ban them from balancing.


Add a config to allow overriding collections retrieval methods

Could you expand a bit on the use case and why it should be in formtastic itself?

Hello @mikz,
Thank you for your answer.
I'm not sure it should be in formtastic, but I found no way to actually cover this case for my project and I did it like that. I'm sorry I had no time to explain more yesterday.
This feature is useful when you need to override how collections are retrieved by formtastic, let's say you have a Post model :
class Post
  has_one :company
  belongs_to :user
end

a User model
class User
  has_many :posts
  belongs_to :company
end

a Company model
class Company
  has_many :users
  has_many :posts, through: :users
end

You may be designing a form to allow a user to add a post for anybody else in his company.
There is today a way to do that using formtastic by directly passing the collection to the input of course:
# Simplified
form do |f|
  f.input :user, collection: proc {User.where(company: current_user.company)}
end

But there is no way of implementing a default behavior for this retrieval. In my case I'm using active admin with cancancan for authorization, and as formtastic handles automatically the retrieval, even if I do not want one of my user to see some associated records, he sees them all unless I specify the collection to every single input of every form of the app.
I also thought it would be a nice way to allow gems depending on formtastic (activeadmin included) to actually be able to override that part of the form. I may write another PR on activeadmin later to be able to scope the filters and form inputs of active admin to be scoped to the AuthorizationAdapter they let the user create.
For my case, I added that in my initializer
# config/initializers/formtastic.rb
Formtastic::FormBuilder.base_input_collections_module_override = CustomBaseForActiveAdmin

And I overrided the collection_from_association method as I wanted to.
I first tried to let active admin provide this level of customization but I could not do much as every inputs includes a module and not a class, so it's difficult to reopen it and just edit methods, although I may have missed something.
Of course, I'm not qualified enough to know if this should be inside Formtastic or somewhere else, it also depends on the level of customization you want to provide and I'll let you judge if you think it's a nice feature to have.

@Uelb I see. So for overriding collection_from_association in some global way to limit scope of the records returned.
Could you share some of those customisations to see what kind of collaborators it needs?
I'd like to see if that requires just the model or also view/controller/form.
Maybe the solution would be just to specify a scope option.

Sorry for the late answer.
Here is the (simplified) code I use so that every resources scopable to a company is scoped automatically and I ensure there is no issue on what the user is able to see.
module CustomBaseForActiveAdmin
  def self.included(base)
    base.send :alias_method, :old_collection_from_association, :collection_from_association
  end

  def collection_from_association
    if reflection
      if object.class != Ransack::Search && reflection.klass.respond_to?(:with_company)
        agency_id = if object.respond_to?(:agency)
          if object.persisted?
            object.agency.id
          else
            template.current_user.agency_ids
          end
        end
        return super unless agency_id
        super.with_agency(agency_id)
      else
        super
      end
    end
  end
end

What about using a Symbol for the collection option like:
f.input :user, collection: :company_users

Or just using a proc:
f.input :user, collection: &:company_users

And implementing a helper method.
I think using code in all collection inputs does not help readability or clarity in the form definitions and there are better means to achieve the same.
I'd consider implementing FormBuilder#collection_for_reflection(reflection) or even better abstracting it into own class like Formtastic::InputClassFinder or Formtastic::ActionClassFinder and having this object just single responsibility to find a relation for a reflection and some form object.

I see 2 potential issues with your approach :

There is no way to be sure that every input has the wanted behavior, and thus a company would if omitted be able to see other users company.
One has to repeat the collection: ... for every input, and that's a lot of repetition when using formtastic a lot

I'm not sure I understand how this way would be different from today as we can already use a proc and access the current_user for instance inside the proc. The proc can also be a variable so it would work as a helper method.
The point of the pull-request is not to have to do that for every input when you want to implement a global behavior for your form or you have critical data segregation to do.
Finally, I have not understood your last sentence, do you mean that you would consider putting a collection_for_reflection method in another class used in the FormBuilder module ? In that case, it would be easy for any user to extend this class and I guess it would solve the use case. The main issue I had is that most of the code is written in modules so it's more difficult to extend it in "custom code" than if it was a class

Agree that having it in every input can be tedious and can be error prone, but that is why we have tests.
Methods are easier testable than proc in view. Imo there is no need for proc in view (i'd even try to use render collection instead of .each).
But to stick to the point of having a global way to modify how collections are found. I'd consider putting it on the FormBuilder module or even better extracting it into a class and allow configuration like: https://github.com/justinfrench/formtastic/blob/ec0b090de764f09cd7d68177ac6fea9011d907b4/lib/formtastic/form_builder.rb#L48
That is later used: https://github.com/justinfrench/formtastic/blob/b104bb1fe5228ea37f668917b13d0888f8bd033a/lib/formtastic/helpers/action_helper.rb#L107-L109
Then you'd just be able to configure the collection finder globally or per form.
And regarding this PR, I think it can be easily just a monkey patch in your application using Module#prepend:
module MyGlobalModule
  def collection_from_association
    if reflection
      User.where(company: current_user.company)
    else super end
  end
end

Formtastic::Inputs::Base::Collections.prepend(MyGlobalModule)

I think the class and configuration idea is really nice ! Do you have time to implement it or do you want me to do it ?

@Uelb unfortunately I really don't have time to implement it. We have a 4.0 release pending for ages and I struggle to find time for that.

Ok, I'll do my best to submit something that won't take too much time to review

@Uelb could you try to consider other collection_for_ methods like for enum  https://github.com/justinfrench/formtastic/pull/1227 ?
Also I expect some people could have already modified this behaviour, so we should not remove the collection_for_association method, but rather delegate to the new object and possibly print a deprecation warning like was done for the action/input finders (https://github.com/justinfrench/formtastic/pull/1063).

Closing for now due to inactivity.